Add parsing error message field to TrinoQueryProperties

[Chaho12]

Description

Add a field (private Optional ParseErrorMessage) to TrinoQueryProperties class that stores e.getMessage() when there is error.
https://github.com/trinodb/trino-gateway/blob/main/gateway-ha/src/main/java/io/trino/gateway/ha/router/TrinoQueryProperties.java#L213-L224

We have server connected with gateway that runs explain before actually running the query.
But not all queries need to be sent with explain.

For example there are cases where queries with parsing failure (no need to run explain cause it is highly likely to fail anyway due to syntax error)

• we need to tell user on WHY query has failed, but running another sql parser or explain is waste of resource
• since gateway already does query parsing, why not use that failure info.

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

[ebyhr]

(x) Release notes are required, with the following suggested text:

• Add parsing error message field to TrinoQueryProperties

Why do we need to mention this change in a release note?

[Chaho12]

Why do we need to mention this change in a release note?

Thought there might be some people like me who would need the error info.
Not necessary though, docs is plenty.

[ebyhr]

Why do we need both isQueryParsingSuccessful and parseErrorMessage variables? Is it possible to remove isQueryParsingSuccessful and return the value by checking the size of parseErrorMessage on the fly?

[Chaho12]

So if there isn't an errorMessage than it is considered successful?
Hmm.. I prefer having a separate variable. @willmostly any thoughts?

[ebyhr]

When does isQueryParsingSuccessful become true when parseErrorMessage is present?
When does isQueryParsingSuccessful become false when parseErrorMessage is empty?

Having two variables is troublesome as it's easy to miss updating another one. Why do you prefer having a separate variable?

[Chaho12]

Nowadays am used to using Golang as it always has error field to check before doing any further actions.

• https://go.dev/blog/error-handling-and-go

Quite convenient should i say.
But am ok with just having error message and removing boolean field.

[ebyhr]

Exposing two fields is fine. It's convenient for users. My question was why we need to manage both isQueryParsingSuccessful and parseErrorMessage internally instead of the following:

    public boolean isQueryParsingSuccessful()
    {
        return errorMessage.isEmpty();
    }

[Chaho12]

Any ideas on handling this? because when i do this

    @JsonProperty("isQueryParsingSuccessful")
    public boolean isQueryParsingSuccessful()
    {
        return errorMessage.isEmpty();
    }

I get this error. As of now, isQueryParsingSuccessful is assigned but never accessed.
There is no need to access the field in code, but i don't see any other better ways than simply adding @SuppressWarnings("unused")

gateway-ha/src/main/java/io/trino/gateway/ha/router/TrinoQueryProperties.java:99:21
java: [UnusedVariable] The field 'isQueryParsingSuccessful' is never read.

[Chaho12]

@ebyhr Thanks for feedback. I think i've finished addressing your points. PTAL :)

[ebyhr]

Exposing two fields is fine. It's convenient for users. My question was why we need to manage both isQueryParsingSuccessful and parseErrorMessage internally instead of the following:

    public boolean isQueryParsingSuccessful()
    {
        return errorMessage.isEmpty();
    }

[mosabua]

Looks like @ebyhr and @Chaho12 are closing this out together shortly. Let me know if you want a review or other help from me.

[ebyhr]

Suggested change

	assertThat(deserializedTrinoQueryProperties.isQueryParsingSuccessful()).isEqualTo(trinoQueryProperties.isQueryParsingSuccessful());;
	assertThat(deserializedTrinoQueryProperties.isQueryParsingSuccessful()).isEqualTo(trinoQueryProperties.isQueryParsingSuccessful());

[ebyhr]

Why do we still need this field? I thought just having the getter is sufficient.