[
https://issues.apache.org/jira/browse/CALCITE-3122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16882480#comment-16882480
]
Julian Hyde commented on CALCITE-3122:
--------------------------------------
I've reviewed the PR. Despite the long list of comments below, I think the code
is in pretty good shape, and we're close to being able to commit.
Review comments:
* Can you make {{Driver.CONNECT_STRING_PREFIX}} immutable again? Perhaps you
need to sub-class the {{Driver}}.
* Can you make {{RelOptCluster.planner}} final again?
* You added method {{RelToSqlConverter.visit(Correlate)}}, which is great - it
will be useful for other people - but can you please add a test to
{{RelToSqlConverterTest}}?
* It would be nice to add {{Uncollect}} to {{RelBuilder}}, and also an
{{UncollectFactory}}; can you please log a follow-on Jira
* {{SqlImplementor.resetAlias}} needs a javadoc comment
* You added {{RexWindowBound.toSqlNode}} but I think that is too much coupling
between rex and sql packages. Can you move the logic into
{{SqlImplementor.Context}}? There are already several {{toSql}} methods there.
* {{ScalarFunctionImpl.createUnsafe}} needs javadoc
* {{SqlNode.toSqlString}} doesn't seem quite right. If you re-use the same
writer for several nodes, on the second node it will contain the SQL for the
first and second nodes, right? I think you should remove the method and just
use {{unparse}} directly..
* {{PigRelBuilder.group}} needs javadoc
* In {{RelBuilder}} you added methods {{getCluster()}}, {{getRelOptSchema()}},
{{getScanFactory()}}. Those fields should remain private (or protected), so I
don't want to add those methods.
* The tests in the piglet module produce a lot of warnings. Is there a way to
silence pig's tracing?
* Make {{DynamicTupleRecordType.nameToIndex}} static, and store the
pre-compiled regex pattern in a static final field.
* Do we need {{PigRelBuilder}} and {{LogicalPigRelBuilder}} to be separate
classes? I don't understand what their separate purposes are.
* {{ToLogicalConverter}} would be more robust and flexible if you used a
{{RelBuilder}} rather than calling {{RelNode.create}} methods explicitly
* What is "dummy" about a PigDummyTable? It does as much as most other Table
implementations in Calcite.
* What do you mean by "PigRel"? (It occurs at several points in the doc but is
not defined.) Similarly "PigRelEx".
* In {{replacePatternIfPossible}}, remove the commented-out code and log a JIRA
case for the functionality that is missing.
* In {{PigUdfWrapper}}, re-visit "TODO: Fix the enumerable code generator or
auto-generate code for this wrapper"
While reviewing, I also did a lot of "fix up" (mainly cosmetic, remove a few
calls to deprecated methods, rename a few classes to fit Calcite's conventions,
rebase, etc.). You can see the results of the fix up in
[https://github.com/julianhyde/calcite/tree/3122-pig] but you should continue
adding commits to your branch and we can squash/rebase later.
> Convert Pig Latin scripts into Calcite logical plan
> ----------------------------------------------------
>
> Key: CALCITE-3122
> URL: https://issues.apache.org/jira/browse/CALCITE-3122
> Project: Calcite
> Issue Type: New Feature
> Components: core, piglet
> Reporter: Khai Tran
> Priority: Major
> Labels: pull-request-available
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> We create an internal Calcite repo at LinkedIn and develop APIs to parse any
> Pig Latin scripts into Calcite logical plan. The code was tested in nearly
> ~1000 Pig scripts written at LinkedIn.
> Changes:
> 1. piglet: main conversion code live there, include:
> * APIs to convert any Pig scripts into RelNode plans or SQL statements
> * Use Pig Grunt parser to parse Pig Latin scripts into Pig logical plan
> (DAGs)
> * Convert Pig schemas into RelDatatype
> * Traverse through Pig expression plan and convert Pig expressions into
> RexNodes
> * Map some basic Pig UDFs to Calcite SQL operators
> * Build Calcite UDFs for any other Pig UDFs, including UDFs written in both
> Java and Python
> * Traverse (DFS) through Pig logical plans to convert each Pig logical nodes
> to RelNodes
> * Have an optimizer rule to optimize Pig group/cogroup into Aggregate
> operators
> 2. core:
> * Implement other RelNode in Rel2Sql so that Pig can be translated into SQL
> * Other minor changes in a few other classes to make Pig to Calcite works
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
