walterddr commented on PR #8880: URL: https://github.com/apache/pinot/pull/8880#issuecomment-1156546397
> Actually after a second thought, I think it is okay to keep the current way without affecting current query options. The only query option value that is not number/boolean is `FORCE_HLC` which I believe no one is using. @siddharthteotia adding `OPTION(identifier = identifier)` is possible but the identifier cannot start with a number so we are still not backward-compatible. > > There is a major issue not handled though. In `PinotClientRequest.constructSqlQueryOptions()`, the `groupByMode` and `responseFormat` value `SQL` cannot be parsed, and that can cause user not able to upgrade from 0.10.0. We need to quote the value if there is no easy way to support the previous format This is a good catch, will fix; but also do we know if there's any other places that adds the OPTION regex to the query? > Just noticed that seems it requires an extra ; to split the statements, then all the existing query option won't work... We should document this change in the PR description Yes this is the standard SQL statement separator. however the `;` before the `<EOF>` token can be skipped thus we don't have any issue so far. I will document this. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
