[GitHub] [pinot] walterddr commented on pull request #8880: query option to use parser.jj extension instead of regex

[GitBox <gi...@apache.org>]

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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org