Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21194 )
Change subject: IMPALA-12934: Added Calcite parsing files to Impala ...................................................................... Patch Set 9: (2 comments) I saw your long comment and your follow-up comment. I assume from your follow-up comment that we should stick with option 2? My take was this: If we were purely making changes to config.fmpp, option 1 makes more sense. But since Parser.jj has changes, I figured it makes sense to have all of the parsing information in front of us as opposed to going back and forth. I don't have a super strong feeling about it, so let me know if we should file a Jira to at least revisit what we want to do. As for putting it under the codegen directory...As you can see, I made the change, and I suppose it makes sense. It's not really a "java" file, so to put it under src/main/java has a slightly wrong feel to it. My one comment though is this: When I first started the Impala project, I did a lot of my searching within a bash shell under the src/main/java/... directory. THe "codegen" directory feels to me like it's in a side little nook. It took me a little extra time to find that code when I was trying to understand how the code works. So I'm not in love with having this code organization. But now that I'm used to it, and now that I suppose many other projects work this way, it does make sense to me to put it in the location you suggested. http://gerrit.cloudera.org:8080/#/c/21194/9/java/calcite-planner/pom.xml File java/calcite-planner/pom.xml: http://gerrit.cloudera.org:8080/#/c/21194/9/java/calcite-planner/pom.xml@106 PS9, Line 106: <plugin> : <groupId>org.apache.maven.plugins</groupId> : <artifactId>maven-resources-plugin</artifactId> : <executions> : <execution> <!-- copy all templates/data in the same location to compile them at once --> : <id>copy-resources</id> : <phase>generate-sources</phase> : <goals> : <goal>copy-resources</goal> : </goals> : <configuration> : <outputDirectory>${project.build.directory}/codegen</outputDirectory> : <resources> : <resource> : <directory>src/main/java/org/apache/impala/calcite/parserimpl/codegen</directory> : <filtering>false</filtering> : </resource> : </resources> : </configuration> : </execution> : </executions> : </plugin> > After a second look, I think I would put the Parser.jj and config.fmpp in s I did have to put Parser.jj under a separate directory "templates" because it needs the "jj" files in its own separate directory. It's very sensitive...even the existence of a ".Parser.jj.swp" file in that directory will be picked up and cause problems. But this also matches the directory structure in Calcite. http://gerrit.cloudera.org:8080/#/c/21194/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/parserimpl/codegen/config.fmpp File java/calcite-planner/src/main/java/org/apache/impala/calcite/parserimpl/codegen/config.fmpp: http://gerrit.cloudera.org:8080/#/c/21194/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/parserimpl/codegen/config.fmpp@21 PS9, Line 21: package: "org.apache.calcite.sql.parser.impl", > Let's make this somewhere in org.apache.impala. Done -- 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: 9 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: Tue, 07 May 2024 16:51:48 +0000 Gerrit-HasComments: Yes
