Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21194 )
Change subject: IMPALA-12934: Added Calcite parsing files to Impala ...................................................................... Patch Set 8: Code-Review+1 (1 comment) This change looks good to me. I'm going to kick off tests. http://gerrit.cloudera.org:8080/#/c/21194/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21194/8//COMMIT_MSG@18 PS8, Line 18: The config.fmpp file was grabbed from Calcite 1.36 default_config.fmpp. It does : have two very small modifications from the original Calcite fmpp file in order : to fix compilation issues within Impala : : 1) the entire json is wrapped with a "data {}" tag : 2) the class used is ImpalaSqlParserImpl as opposed to the Calcite SqlParserImpl : class to prevent naming collisions with Calcite. Let me check my understanding: - The config.fmpp has a particular structure that we need to follow. We're constructing a config.fmpp following the same structure as https://github.com/apache/calcite/blob/main/core/src/main/codegen/config.fmpp. - The default values for the "parser" section come from default_config.fmpp. We could have a minimal config.fmpp with just the parser class name if we wanted to. - To make it easier to understand future Impala modifications we make, we are pulling in all of the settings from default_config.fmpp to this config.fmpp's parser section. If that description is correct, then I think we could edit this section and say that we're constructing a config.fmpp in its usual format with our custom class name but including all the content from default_config.fmpp to make it easier to understand future changes. -- To view, visit http://gerrit.cloudera.org:8080/21194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If756b5ea8beb85661a30fb5d029e74ebb6719767 Gerrit-Change-Number: 21194 Gerrit-PatchSet: 8 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Wed, 01 May 2024 00:00:01 +0000 Gerrit-HasComments: Yes
