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 9:
(2 comments)
So, I went down a rabbit hole trying to understand this. Basically, there are
two options. Option 1 is finding some way to pull in Calcite's
default_config.fmpp to our build and config.fmpp contains only the modified
values. Option 2 is to specify every value from default_config.fmpp in
config.fmpp and skip pulling in default_config.fmpp. This does option 2.
The way default_config.fmpp comes in is that Parser.jj has many references to
default.parser.X as the value to use if parser.X doesn't exist. i.e.
<#list (parser.imports!default.parser.imports) as importStr>
import ${importStr};
</#list>
Calcite has a custom gradle plugin that incorporates default_config.fmpp into
the fmpp compilation. Basically, it is adding a link to a tdd file from the
data section as "default". So, it is equivalent to something like this:
data {
parser {
...
}
default: tdd("../default_config.fmpp")
}
Dremio does something similar with a Maven plugin. Everything in
default_config.fmpp is in the parser section, so it's values end up available
as default.parser.X.
In Option 2, parser.X always exists, so it doesn't matter that we never hooked
up default_config.fmpp to have the default.parser.X value available. I can live
with that, but see my comments about moving things to src/main/codegen and
using the Impala package.
All that said, I think Option 1 is easier to understand and I think it is
pretty easy. Basically, we pull in default_config.fmpp from Calcite, then add
that "default: tdd("../default_config.fmpp")" line to our config.fmpp after the
close of the parser section (the path has a .. because I guess it is evaluated
from the templates/ directory). After that, our config.fmpp only needs to
contain the things we modify. default_config.fmpp stays identical the Calcite
file. (Dremio does a slick thing where they pull default_config.fmpp out of the
Calcite jar. We could do that at some point, but I don't feel compelled to do
that immediately.)
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
src/main/codegen and skip this step. This is the organization that Calcite has,
and I think it makes sense.
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.
--
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: Mon, 06 May 2024 23:48:56 +0000
Gerrit-HasComments: Yes
