[
https://issues.apache.org/jira/browse/CALCITE-2454?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16741793#comment-16741793
]
Vladimir Sitnikov edited comment on CALCITE-2454 at 1/14/19 7:14 AM:
---------------------------------------------------------------------
{quote}VolcanoPlanner.key() is obsolete{quote}
Indeed.
{quote}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)
also 'c' is unused, which means you have a bug
{quote}
done
{quote}In RexCall.equalSansNullability you need some javadoc, and also explain
why it is better/different than RelDataTypeFactory.equalSansNullability;{quote}
We don't have RelDataTypeFactory in the RexLiteral constructor. Frankly
speaking I would love to have {{RelDataType#equalIgnoreNullability}}, however
it would alter public API.
{quote}Can you explain why RexLiteral.typeOmitted is necessary? It doesn't feel
like a field, even though it is derived from other fields. I see it is used in
some complex logic in RexCall.appendOperands which also calls the broken
equalSansNullability method.{quote}
typeOmitted was there to cache the decision of shouldIncludeType. However, I
don't need that cache assuming NlsString#getValue caches string representation.
So I removed the field at a cost of more calls to shouldIncludeType
{quote}In RexLiteral.shouldIncludeType needs javadoc.{quote}
{quote}Also, please improve the indent of the complex 'if' condition to improve
readability.{quote}
[~julianhyde], This one puzzles me. Can you please check if the updated code is
what you want?
{quote}It doesn't seem right to add a RexDigestIncludeType argument to
printAsJava.{quote}
It is required for Multiset case (~Piglet adapter) when RexLiteral's value is
List<RexLiteral>, so we want to propagate "includeDigest" for those RexLiterals
as well.
{quote}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.{quote}
I don't really know coercibility rules, however it seems that coercibility
strength should be adjusted based on the context. There might be a valid reason
for "new" there, however the change produces the same results (modulo object
identity which we shouldn't really care about).
{quote}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.{quote}
The de-facto source of truths is Travis, so I postpone things like
120-second-long LatticeTest. Especially taking into account that LatticeTest
has never ever caught a bug in a PR.
was (Author: vladimirsitnikov):
{quote}VolcanoPlanner.key() is obsolete{quote}
Indeed.
{quote}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)
also 'c' is unused, which means you have a bug
{quote}
done
{quote}In RexCall.equalSansNullability you need some javadoc, and also explain
why it is better/different than RelDataTypeFactory.equalSansNullability;{quote}
We don't have RelDataTypeFactory in the RexLiteral constructor. Frankly
speaking I would love to have {{RelDataType#equalIgnoreNullability}}, however
it would alter public API.
{quote}Can you explain why RexLiteral.typeOmitted is necessary? It doesn't feel
like a field, even though it is derived from other fields. I see it is used in
some complex logic in RexCall.appendOperands which also calls the broken
equalSansNullability method.{quote}
I've changed the field to have RexDigestIncludesType for improved readability.
The thing is RexLiteral computes type=OPTIONAL digest at creation, and we want
to optimize type=ALWAYS as well in case the computed digest includes type.
{quote}In RexLiteral.shouldIncludeType needs javadoc.{quote}
{quote}Also, please improve the indent of the complex 'if' condition to improve
readability.{quote}
[~julianhyde], This one puzzles me. Can you please check if the updated code is
what you want?
{quote}It doesn't seem right to add a RexDigestIncludeType argument to
printAsJava.{quote}
It is required for Multiset case (~Piglet adapter) when RexLiteral's value is
List<RexLiteral>, so we want to propagate "includeDigest" for those RexLiterals
as well.
{quote}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.{quote}
I don't really know coercibility rules, however it seems that coercibility
strength should be adjusted based on the context. There might be a valid reason
for "new" there, however the change produces the same results (modulo object
identity which we shouldn't really care about).
{quote}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.{quote}
The de-facto source of truths is Travis, so I postpone things like
120-second-long LatticeTest. Especially taking into account that LatticeTest
has never ever caught a bug in a PR.
> 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)
