Skip to content

Add type information to RexLiteral#digest, ensure planner uses just RelNode#digest for identity#1002

Closed
vlsi wants to merge 1 commit intoapache:masterfrom
vlsi:rexliteral_digest
Closed

Add type information to RexLiteral#digest, ensure planner uses just RelNode#digest for identity#1002
vlsi wants to merge 1 commit intoapache:masterfrom
vlsi:rexliteral_digest

Conversation

@vlsi
Copy link
Contributor

@vlsi vlsi commented Jan 11, 2019

No description provided.

@vlsi
Copy link
Contributor Author

vlsi commented Jan 11, 2019

Unfortunately, it adds types to each and every value for Values.tuple:

I wonder if we can add special "digest without type" for Values puposes (it can print types just once)

planBefore expected:<...ion=[>(+($0, $1), 50[)])
    LogicalValues(tuples=[[{ 10, 1 }, { 30, 3] }]])
> but was:<...ion=[>(+($0, $1), 50[:INTEGER NOT NULL)])
    LogicalValues(tuples=[[{ 10:INTEGER NOT NULL, 1:INTEGER NOT NULL }, { 30:INTEGER NOT NULL, 3:INTEGER NOT NULL] }]])
>

On the other hand, null expressions become easier to understand:

Expected: "AND(null, IS NULL(?0.a))"
     but: was "AND(null:BOOLEAN, IS NULL(?0.a))"

@vlsi vlsi force-pushed the rexliteral_digest branch 5 times, most recently from ae54d51 to d496906 Compare January 13, 2019 16:46
@vlsi vlsi changed the title WIP: try adding type information to RexLiteral.digest Add type information to RexLiteral#digest, ensure planner uses just RelNode#digest for identity Jan 13, 2019
case COERCIBLE:
switch (coercibility2) {
case COERCIBLE:
return new SqlCollation(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@julianhyde , I don't quite get new SqlCollation thing here, however it looks like new here makes no difference. It might be SqlCollation#strength should be updated as well, however it does not seem to be supported in Calcite yet.

This change was required so SqlCollation.IMPLICIT singleton is propagated across all expressions.

@vlsi vlsi force-pushed the rexliteral_digest branch from ba17d23 to d3166e7 Compare January 14, 2019 06:25
@vlsi vlsi added LGTM-will-merge-soon Overall PR looks OK. Only minor things left. and removed help wanted labels Feb 5, 2019
@vlsi vlsi force-pushed the rexliteral_digest branch from bf3bcc9 to b56cb1b Compare February 6, 2019 16:41
@asfgit asfgit closed this in 866d855 Feb 6, 2019
jszeluga pushed a commit to jszeluga/calcite that referenced this pull request Feb 18, 2019
…case the type of 1 is int in the first rel and long in the second one

Add type information to RexLiteral#digest, ensure planner uses just RelNode#digest for identity

fixes apache#1002
zhztheplayer pushed a commit to zhztheplayer/calcite that referenced this pull request Feb 20, 2019
…case the type of 1 is int in the first rel and long in the second one

Add type information to RexLiteral#digest, ensure planner uses just RelNode#digest for identity

fixes apache#1002
chadasa pushed a commit to chadasa/calcite that referenced this pull request Feb 27, 2019
…case the type of 1 is int in the first rel and long in the second one

Add type information to RexLiteral#digest, ensure planner uses just RelNode#digest for identity

fixes apache#1002
wangxlong pushed a commit to wangxlong/calcite that referenced this pull request Feb 13, 2020
…case the type of 1 is int in the first rel and long in the second one

Add type information to RexLiteral#digest, ensure planner uses just RelNode#digest for identity

fixes apache#1002
jamesstarr pushed a commit to jamesstarr/calcite that referenced this pull request Aug 28, 2025
…case the type of 1 is int in the first rel and long in the second one

Add type information to RexLiteral#digest, ensure planner uses just RelNode#digest for identity

fixes apache#1002

Change-Id: I00ff6e0d6aed08d1beecb85e4508444db6e15248
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant