[
https://issues.apache.org/jira/browse/CALCITE-2454?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16741684#comment-16741684
]
Julian Hyde commented on CALCITE-2454:
--------------------------------------
Review comments:
* VolcanoPlanner.key() is obsolete
* In Values.java, there are a couple of calls to pw.nest() that will always
return false
* Make RexCall.SIMPLE_OPS private and immutable (Guava's ImmutableEnumSet would
do the trick)
* In RexCall.equalSansNullability you need some javadoc, and also explain why
it is better/different than RelDataTypeFactory.equalSansNullability; also 'c'
is unused, which means you have a bug
* Can you explain why RexLiteral.typeOmitted is necessary? It doesn't feel like
a field, because it's not part of the identity of a literal, more like the
context in which that literal is used. Since RexNodes (including RexLiterals)
can be used multiple times, their identity must not contain their usage context.
* In RexLiteral, shouldIncludeType needs javadoc. Please improve the indent of
the complex 'if' condition to improve readability.
* It doesn't seem right to add a RexDigestIncludeType argument to printAsJava.
* In SqlCollation.getCoercibilityDyadic, I agree with your changes. I'm not
sure why the code used to do 'new'. Maybe someone was worried about mutability.
* You are technically correct in moving slow tests such as LatticeTest and
JdbcTest to the end of the list in CalciteSuite. However, in a multi-threaded
environment that may make the whole suite take longer.
Can you clarify: when you omit charset and collation, are you using the SESSION
defaults or the GLOBAL default? It's important that you use the GLOBAL default,
because may need to compare digests that are created in sessions that have
dfiferent default charsets.
> HepPlanner should not pick cached RelNode when type mismatch while digest
> matches
> ---------------------------------------------------------------------------------
>
> Key: CALCITE-2454
> URL: https://issues.apache.org/jira/browse/CALCITE-2454
> Project: Calcite
> Issue Type: Bug
> Components: core
> Affects Versions: 1.17.0
> Reporter: Yuzhao Chen
> Assignee: Julian Hyde
> Priority: Major
> Labels: pull-request-available
> Attachments: expr_types.png, project_with_literals_digest.png
>
>
> After CALCITE-2116, a RelNode will be reused based on digest, but now many
> nodes's digests do not have type info, e.g. LogicalProject, this will affect
> some rules when we wanna transform to a node with same digest and same type
> (HepPlanner may pick out a RelNode with different type).
> So, we should add type info just like VolcanoPlanner.
> We encounter this problem with plan like below:
> {code:java}
> Union
> +-- project1(a: int, b:int, c: double)
> | +-- project2(a: bigint: b: int, c:double)
> +-- project3
> {code}
> We wanna cast a from bigint -> int so we can have a union with project1 and
> project3, but with ProjectMergeRule, while merging project1 and project2,
> HepPlanner will think project2 and project1 are identical and it gives back
> project2. But actually we wanna pick project1.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
