[jira] [Commented] (CALCITE-6122) In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
[
https://issues.apache.org/jira/browse/CALCITE-6122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17788457#comment-17788457
]
Steve Carlin commented on CALCITE-6122:
---
[~shenlang] , my latest comment has the repro
> In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
> ---
>
> Key: CALCITE-6122
> URL: https://issues.apache.org/jira/browse/CALCITE-6122
> Project: Calcite
> Issue Type: Bug
> Components: core
>Affects Versions: 1.36.0
>Reporter: Steve Carlin
>Priority: Major
>
> I hope I'm describing this right.
> I'm coercing an operand in my handmade OperandTypeChecker. Specifically, I'm
> changing a {color:#de350b}SUM(tinyint_col){color} to a
> {color:#de350b}SUM(CAST(tinyint_col as BIGINT)){color} because my database
> can only handle a bigint operand.
> The eventual logical plan does not keep the CAST operand.
> I had this problem in 1.34.0. I noticed that the code changed quite a bit
> going up to 1.36.0, so I say this to mention that this is not a regression.
> I hacked a fix in my environment, but it's too hacky to commit. To fix the
> problem, I changed to the following code in SqlToRelConverter.convertAgg():
>
> {code:java}
> @@ -3369,7 +3372,11 @@ protected void convertAgg(Blackboard bb, SqlSelect
> select,
> final AggConverter aggConverter =
> AggConverter.create(bb,
> (AggregatingSelectScope) validator().getSelectScope(select));
> - createAggImpl(bb, aggConverter, selectList, groupList, having,
> + selectList.accept(aggConverter);
> + final AggConverter aggConverter2 =
> + AggConverter.create(bb,
> + (AggregatingSelectScope) validator().getSelectScope(select));
> + createAggImpl(bb, aggConverter2, selectList, groupList, having,
> orderExprList);
> }
> {code}
> Note that I had the selectList go through the aggConverter visitor. After
> this, the selectList contains the coerced operands. If the aggConverter is
> created based on this new selectList, it will contain the proper information
> in the aggConverter.convertedInputExprs list
> (One other note: There is an assertion in createAggImpl that i had to disable
> in order to get this hack to work where it checks that bb.agg == null)
> I can probably work on this, but I'm not sure how to create a proper test for
> it, as I've never committed anything to Calcite before.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Reopened] (CALCITE-6122) In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
[
https://issues.apache.org/jira/browse/CALCITE-6122?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Steve Carlin reopened CALCITE-6122:
---
Reopening this now that I've had time to investigate further.
I have two unit tests that I added to SqlValidatorTest that showcase the
problem:
{code:java}
@Test void testShouldHaveCast() {
sql("select sum(ename) from emp")
.withValidatorIdentifierExpansion(false)
.rewritesTo("SELECT SUM(`ENAME`)\n"
+ "FROM `EMP`");
}
@Test void testCorrectWithCast() {
sql("select sum(ename) from emp")
.withValidatorIdentifierExpansion(true)
.rewritesTo("SELECT SUM(CAST(`EMP`.`ENAME` AS DECIMAL(19, 9)))\n"
+ "FROM `CATALOG`.`SALES`.`EMP` AS `EMP`");
}
{code}
The "identifierExpansion" config parameter has the side effect of also allowing
type coercion to be sent back with the validated node.
I think I can live with this as a workaround for my code, but it's not great.
I also don't know how easy this will be to fix given that it involves a mutated
selectList parameter in SqlValidatorImpl which gets changed by the
identifierExpansion config param. That code can be found here (in
SqlValidatorImpl.java):
{code:java}
4663 // Create the new select list with expanded items. Pass through
4664 // the original parser position so that any overall failures can
4665 // still reference the original input text.
4666 SqlNodeList newSelectList =
4667 new SqlNodeList(expandedSelectItems,
selectItems.getParserPosition());
4668 if (config.identifierExpansion()) {
4669 select.setSelectList(newSelectList);
4670 }
{code}
> In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
> ---
>
> Key: CALCITE-6122
> URL: https://issues.apache.org/jira/browse/CALCITE-6122
> Project: Calcite
> Issue Type: Bug
> Components: core
>Affects Versions: 1.36.0
>Reporter: Steve Carlin
>Priority: Major
>
> I hope I'm describing this right.
> I'm coercing an operand in my handmade OperandTypeChecker. Specifically, I'm
> changing a {color:#de350b}SUM(tinyint_col){color} to a
> {color:#de350b}SUM(CAST(tinyint_col as BIGINT)){color} because my database
> can only handle a bigint operand.
> The eventual logical plan does not keep the CAST operand.
> I had this problem in 1.34.0. I noticed that the code changed quite a bit
> going up to 1.36.0, so I say this to mention that this is not a regression.
> I hacked a fix in my environment, but it's too hacky to commit. To fix the
> problem, I changed to the following code in SqlToRelConverter.convertAgg():
>
> {code:java}
> @@ -3369,7 +3372,11 @@ protected void convertAgg(Blackboard bb, SqlSelect
> select,
> final AggConverter aggConverter =
> AggConverter.create(bb,
> (AggregatingSelectScope) validator().getSelectScope(select));
> - createAggImpl(bb, aggConverter, selectList, groupList, having,
> + selectList.accept(aggConverter);
> + final AggConverter aggConverter2 =
> + AggConverter.create(bb,
> + (AggregatingSelectScope) validator().getSelectScope(select));
> + createAggImpl(bb, aggConverter2, selectList, groupList, having,
> orderExprList);
> }
> {code}
> Note that I had the selectList go through the aggConverter visitor. After
> this, the selectList contains the coerced operands. If the aggConverter is
> created based on this new selectList, it will contain the proper information
> in the aggConverter.convertedInputExprs list
> (One other note: There is an assertion in createAggImpl that i had to disable
> in order to get this hack to work where it checks that bb.agg == null)
> I can probably work on this, but I'm not sure how to create a proper test for
> it, as I've never committed anything to Calcite before.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (CALCITE-6122) In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
[
https://issues.apache.org/jira/browse/CALCITE-6122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17787645#comment-17787645
]
Steve Carlin commented on CALCITE-6122:
---
Closing this. I think I see how I can do it now.
> In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
> ---
>
> Key: CALCITE-6122
> URL: https://issues.apache.org/jira/browse/CALCITE-6122
> Project: Calcite
> Issue Type: Bug
> Components: core
>Affects Versions: 1.36.0
>Reporter: Steve Carlin
>Priority: Major
>
> I hope I'm describing this right.
> I'm coercing an operand in my handmade OperandTypeChecker. Specifically, I'm
> changing a {color:#de350b}SUM(tinyint_col){color} to a
> {color:#de350b}SUM(CAST(tinyint_col as BIGINT)){color} because my database
> can only handle a bigint operand.
> The eventual logical plan does not keep the CAST operand.
> I had this problem in 1.34.0. I noticed that the code changed quite a bit
> going up to 1.36.0, so I say this to mention that this is not a regression.
> I hacked a fix in my environment, but it's too hacky to commit. To fix the
> problem, I changed to the following code in SqlToRelConverter.convertAgg():
>
> {code:java}
> @@ -3369,7 +3372,11 @@ protected void convertAgg(Blackboard bb, SqlSelect
> select,
> final AggConverter aggConverter =
> AggConverter.create(bb,
> (AggregatingSelectScope) validator().getSelectScope(select));
> - createAggImpl(bb, aggConverter, selectList, groupList, having,
> + selectList.accept(aggConverter);
> + final AggConverter aggConverter2 =
> + AggConverter.create(bb,
> + (AggregatingSelectScope) validator().getSelectScope(select));
> + createAggImpl(bb, aggConverter2, selectList, groupList, having,
> orderExprList);
> }
> {code}
> Note that I had the selectList go through the aggConverter visitor. After
> this, the selectList contains the coerced operands. If the aggConverter is
> created based on this new selectList, it will contain the proper information
> in the aggConverter.convertedInputExprs list
> (One other note: There is an assertion in createAggImpl that i had to disable
> in order to get this hack to work where it checks that bb.agg == null)
> I can probably work on this, but I'm not sure how to create a proper test for
> it, as I've never committed anything to Calcite before.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Closed] (CALCITE-6122) In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
[
https://issues.apache.org/jira/browse/CALCITE-6122?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Steve Carlin closed CALCITE-6122.
-
Resolution: Fixed
> In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
> ---
>
> Key: CALCITE-6122
> URL: https://issues.apache.org/jira/browse/CALCITE-6122
> Project: Calcite
> Issue Type: Bug
> Components: core
>Affects Versions: 1.36.0
>Reporter: Steve Carlin
>Priority: Major
>
> I hope I'm describing this right.
> I'm coercing an operand in my handmade OperandTypeChecker. Specifically, I'm
> changing a {color:#de350b}SUM(tinyint_col){color} to a
> {color:#de350b}SUM(CAST(tinyint_col as BIGINT)){color} because my database
> can only handle a bigint operand.
> The eventual logical plan does not keep the CAST operand.
> I had this problem in 1.34.0. I noticed that the code changed quite a bit
> going up to 1.36.0, so I say this to mention that this is not a regression.
> I hacked a fix in my environment, but it's too hacky to commit. To fix the
> problem, I changed to the following code in SqlToRelConverter.convertAgg():
>
> {code:java}
> @@ -3369,7 +3372,11 @@ protected void convertAgg(Blackboard bb, SqlSelect
> select,
> final AggConverter aggConverter =
> AggConverter.create(bb,
> (AggregatingSelectScope) validator().getSelectScope(select));
> - createAggImpl(bb, aggConverter, selectList, groupList, having,
> + selectList.accept(aggConverter);
> + final AggConverter aggConverter2 =
> + AggConverter.create(bb,
> + (AggregatingSelectScope) validator().getSelectScope(select));
> + createAggImpl(bb, aggConverter2, selectList, groupList, having,
> orderExprList);
> }
> {code}
> Note that I had the selectList go through the aggConverter visitor. After
> this, the selectList contains the coerced operands. If the aggConverter is
> created based on this new selectList, it will contain the proper information
> in the aggConverter.convertedInputExprs list
> (One other note: There is an assertion in createAggImpl that i had to disable
> in order to get this hack to work where it checks that bb.agg == null)
> I can probably work on this, but I'm not sure how to create a proper test for
> it, as I've never committed anything to Calcite before.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] (CALCITE-6122) In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
[ https://issues.apache.org/jira/browse/CALCITE-6122 ]
Steve Carlin deleted comment on CALCITE-6122:
---
was (Author: scarlin):
Pretty sure it's still a problem, though I still need a unit test. The unit
test in TypeCoercion only runs it through the Validator. This is a specific
SqlToRelConverter problem because it does coerce properly, it just does so too
late.
> In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
> ---
>
> Key: CALCITE-6122
> URL: https://issues.apache.org/jira/browse/CALCITE-6122
> Project: Calcite
> Issue Type: Bug
> Components: core
>Affects Versions: 1.36.0
>Reporter: Steve Carlin
>Priority: Major
>
> I hope I'm describing this right.
> I'm coercing an operand in my handmade OperandTypeChecker. Specifically, I'm
> changing a {color:#de350b}SUM(tinyint_col){color} to a
> {color:#de350b}SUM(CAST(tinyint_col as BIGINT)){color} because my database
> can only handle a bigint operand.
> The eventual logical plan does not keep the CAST operand.
> I had this problem in 1.34.0. I noticed that the code changed quite a bit
> going up to 1.36.0, so I say this to mention that this is not a regression.
> I hacked a fix in my environment, but it's too hacky to commit. To fix the
> problem, I changed to the following code in SqlToRelConverter.convertAgg():
>
> {code:java}
> @@ -3369,7 +3372,11 @@ protected void convertAgg(Blackboard bb, SqlSelect
> select,
> final AggConverter aggConverter =
> AggConverter.create(bb,
> (AggregatingSelectScope) validator().getSelectScope(select));
> - createAggImpl(bb, aggConverter, selectList, groupList, having,
> + selectList.accept(aggConverter);
> + final AggConverter aggConverter2 =
> + AggConverter.create(bb,
> + (AggregatingSelectScope) validator().getSelectScope(select));
> + createAggImpl(bb, aggConverter2, selectList, groupList, having,
> orderExprList);
> }
> {code}
> Note that I had the selectList go through the aggConverter visitor. After
> this, the selectList contains the coerced operands. If the aggConverter is
> created based on this new selectList, it will contain the proper information
> in the aggConverter.convertedInputExprs list
> (One other note: There is an assertion in createAggImpl that i had to disable
> in order to get this hack to work where it checks that bb.agg == null)
> I can probably work on this, but I'm not sure how to create a proper test for
> it, as I've never committed anything to Calcite before.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (CALCITE-6122) In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
[
https://issues.apache.org/jira/browse/CALCITE-6122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17787637#comment-17787637
]
Steve Carlin commented on CALCITE-6122:
---
Pretty sure it's still a problem, though I still need a unit test. The unit
test in TypeCoercion only runs it through the Validator. This is a specific
SqlToRelConverter problem because it does coerce properly, it just does so too
late.
> In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
> ---
>
> Key: CALCITE-6122
> URL: https://issues.apache.org/jira/browse/CALCITE-6122
> Project: Calcite
> Issue Type: Bug
> Components: core
>Affects Versions: 1.36.0
>Reporter: Steve Carlin
>Priority: Major
>
> I hope I'm describing this right.
> I'm coercing an operand in my handmade OperandTypeChecker. Specifically, I'm
> changing a {color:#de350b}SUM(tinyint_col){color} to a
> {color:#de350b}SUM(CAST(tinyint_col as BIGINT)){color} because my database
> can only handle a bigint operand.
> The eventual logical plan does not keep the CAST operand.
> I had this problem in 1.34.0. I noticed that the code changed quite a bit
> going up to 1.36.0, so I say this to mention that this is not a regression.
> I hacked a fix in my environment, but it's too hacky to commit. To fix the
> problem, I changed to the following code in SqlToRelConverter.convertAgg():
>
> {code:java}
> @@ -3369,7 +3372,11 @@ protected void convertAgg(Blackboard bb, SqlSelect
> select,
> final AggConverter aggConverter =
> AggConverter.create(bb,
> (AggregatingSelectScope) validator().getSelectScope(select));
> - createAggImpl(bb, aggConverter, selectList, groupList, having,
> + selectList.accept(aggConverter);
> + final AggConverter aggConverter2 =
> + AggConverter.create(bb,
> + (AggregatingSelectScope) validator().getSelectScope(select));
> + createAggImpl(bb, aggConverter2, selectList, groupList, having,
> orderExprList);
> }
> {code}
> Note that I had the selectList go through the aggConverter visitor. After
> this, the selectList contains the coerced operands. If the aggConverter is
> created based on this new selectList, it will contain the proper information
> in the aggConverter.convertedInputExprs list
> (One other note: There is an assertion in createAggImpl that i had to disable
> in order to get this hack to work where it checks that bb.agg == null)
> I can probably work on this, but I'm not sure how to create a proper test for
> it, as I've never committed anything to Calcite before.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (CALCITE-6122) In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
[
https://issues.apache.org/jira/browse/CALCITE-6122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17787606#comment-17787606
]
Steve Carlin commented on CALCITE-6122:
---
Thoughts while I was sleeping on this...
It's possible that I'm "abusing the system" since I'm doing type coercion in my
own OperandTypeChecker rather than through the TypeCoercion code. It seems to
work fine on scalar operations, but this is specific for aggregates. I did see
a "select sum(varchar_col) from mytbl" test in the TypeCoercionTest java file,
so it does seem to be working for some code.
Let me investigate this further.
> In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
> ---
>
> Key: CALCITE-6122
> URL: https://issues.apache.org/jira/browse/CALCITE-6122
> Project: Calcite
> Issue Type: Bug
> Components: core
>Affects Versions: 1.36.0
>Reporter: Steve Carlin
>Priority: Major
>
> I hope I'm describing this right.
> I'm coercing an operand in my handmade OperandTypeChecker. Specifically, I'm
> changing a {color:#de350b}SUM(tinyint_col){color} to a
> {color:#de350b}SUM(CAST(tinyint_col as BIGINT)){color} because my database
> can only handle a bigint operand.
> The eventual logical plan does not keep the CAST operand.
> I had this problem in 1.34.0. I noticed that the code changed quite a bit
> going up to 1.36.0, so I say this to mention that this is not a regression.
> I hacked a fix in my environment, but it's too hacky to commit. To fix the
> problem, I changed to the following code in SqlToRelConverter.convertAgg():
>
> {code:java}
> @@ -3369,7 +3372,11 @@ protected void convertAgg(Blackboard bb, SqlSelect
> select,
> final AggConverter aggConverter =
> AggConverter.create(bb,
> (AggregatingSelectScope) validator().getSelectScope(select));
> - createAggImpl(bb, aggConverter, selectList, groupList, having,
> + selectList.accept(aggConverter);
> + final AggConverter aggConverter2 =
> + AggConverter.create(bb,
> + (AggregatingSelectScope) validator().getSelectScope(select));
> + createAggImpl(bb, aggConverter2, selectList, groupList, having,
> orderExprList);
> }
> {code}
> Note that I had the selectList go through the aggConverter visitor. After
> this, the selectList contains the coerced operands. If the aggConverter is
> created based on this new selectList, it will contain the proper information
> in the aggConverter.convertedInputExprs list
> (One other note: There is an assertion in createAggImpl that i had to disable
> in order to get this hack to work where it checks that bb.agg == null)
> I can probably work on this, but I'm not sure how to create a proper test for
> it, as I've never committed anything to Calcite before.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (CALCITE-6122) In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
[
https://issues.apache.org/jira/browse/CALCITE-6122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17787581#comment-17787581
]
Steve Carlin commented on CALCITE-6122:
---
So I'm not feeling great about how to fix this since I don't know the code too
well. I had a fix in 1.34.0 which worked, and it might be the way to go. I
don't feel great about changing the code because it involves the type coercion
code which mutates an object subtly underneath the covers, so it feels a bit
weird. It is line 488 in 1.36.0 in AggConverter.java:
{code:java}
final RelDataType type = validator().deriveType(bb.scope(), call);{code}
The purpose seems to be to get the derived type, but this has the effect of
mutating the call with the latest type coercions, and that is exactly what I
need for my code to work.
If I move this code before the try block on line 442 where it looks through the
call operands, it fixes my problem. But it seems more like a side effect here
to move this call up rather than the actual intention. Perhaps some who knows
this code more than I do can chime in? Not sure who that would be though. The
last person who touched this was [~julianhyde] , but it was just to move the
AggConverter class out of SqlToRelConverter.
> In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
> ---
>
> Key: CALCITE-6122
> URL: https://issues.apache.org/jira/browse/CALCITE-6122
> Project: Calcite
> Issue Type: Bug
> Components: core
>Affects Versions: 1.36.0
>Reporter: Steve Carlin
>Priority: Major
>
> I hope I'm describing this right.
> I'm coercing an operand in my handmade OperandTypeChecker. Specifically, I'm
> changing a {color:#de350b}SUM(tinyint_col){color} to a
> {color:#de350b}SUM(CAST(tinyint_col as BIGINT)){color} because my database
> can only handle a bigint operand.
> The eventual logical plan does not keep the CAST operand.
> I had this problem in 1.34.0. I noticed that the code changed quite a bit
> going up to 1.36.0, so I say this to mention that this is not a regression.
> I hacked a fix in my environment, but it's too hacky to commit. To fix the
> problem, I changed to the following code in SqlToRelConverter.convertAgg():
>
> {code:java}
> @@ -3369,7 +3372,11 @@ protected void convertAgg(Blackboard bb, SqlSelect
> select,
> final AggConverter aggConverter =
> AggConverter.create(bb,
> (AggregatingSelectScope) validator().getSelectScope(select));
> - createAggImpl(bb, aggConverter, selectList, groupList, having,
> + selectList.accept(aggConverter);
> + final AggConverter aggConverter2 =
> + AggConverter.create(bb,
> + (AggregatingSelectScope) validator().getSelectScope(select));
> + createAggImpl(bb, aggConverter2, selectList, groupList, having,
> orderExprList);
> }
> {code}
> Note that I had the selectList go through the aggConverter visitor. After
> this, the selectList contains the coerced operands. If the aggConverter is
> created based on this new selectList, it will contain the proper information
> in the aggConverter.convertedInputExprs list
> (One other note: There is an assertion in createAggImpl that i had to disable
> in order to get this hack to work where it checks that bb.agg == null)
> I can probably work on this, but I'm not sure how to create a proper test for
> it, as I've never committed anything to Calcite before.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (CALCITE-6122) In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
[
https://issues.apache.org/jira/browse/CALCITE-6122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17787007#comment-17787007
]
Steve Carlin commented on CALCITE-6122:
---
If I knew how to write a unit test (and one with its own OperandTypeChecker), I
might be able to do it, but I haven't done that much research into Calcite yet.
> In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
> ---
>
> Key: CALCITE-6122
> URL: https://issues.apache.org/jira/browse/CALCITE-6122
> Project: Calcite
> Issue Type: Bug
> Components: core
>Affects Versions: 1.36.0
>Reporter: Steve Carlin
>Priority: Major
>
> I hope I'm describing this right.
> I'm coercing an operand in my handmade OperandTypeChecker. Specifically, I'm
> changing a {color:#de350b}SUM(tinyint_col){color} to a
> {color:#de350b}SUM(CAST(tinyint_col as BIGINT)){color} because my database
> can only handle a bigint operand.
> The eventual logical plan does not keep the CAST operand.
> I had this problem in 1.34.0. I noticed that the code changed quite a bit
> going up to 1.36.0, so I say this to mention that this is not a regression.
> I hacked a fix in my environment, but it's too hacky to commit. To fix the
> problem, I changed to the following code in SqlToRelConverter.convertAgg():
>
> {code:java}
> @@ -3369,7 +3372,11 @@ protected void convertAgg(Blackboard bb, SqlSelect
> select,
> final AggConverter aggConverter =
> AggConverter.create(bb,
> (AggregatingSelectScope) validator().getSelectScope(select));
> - createAggImpl(bb, aggConverter, selectList, groupList, having,
> + selectList.accept(aggConverter);
> + final AggConverter aggConverter2 =
> + AggConverter.create(bb,
> + (AggregatingSelectScope) validator().getSelectScope(select));
> + createAggImpl(bb, aggConverter2, selectList, groupList, having,
> orderExprList);
> }
> {code}
> Note that I had the selectList go through the aggConverter visitor. After
> this, the selectList contains the coerced operands. If the aggConverter is
> created based on this new selectList, it will contain the proper information
> in the aggConverter.convertedInputExprs list
> (One other note: There is an assertion in createAggImpl that i had to disable
> in order to get this hack to work where it checks that bb.agg == null)
> I can probably work on this, but I'm not sure how to create a proper test for
> it, as I've never committed anything to Calcite before.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (CALCITE-6122) In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
[
https://issues.apache.org/jira/browse/CALCITE-6122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17787006#comment-17787006
]
Steve Carlin commented on CALCITE-6122:
---
That might be a little tough because it involves writing some manual type
coercing code.
But as I mentioned above, I had an OperandTypeChecker coerce the
SUM(tinyint_col) into a SUM(CAST(tinyint_col as BIGINT).
The Logical plan should contain:
LogicalAggregate -> LogicalProject where the LogicalProject contains the CAST.
Instead, only the LogicalAggregate was produced, as if the coerced operand
didn't exist.
> In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
> ---
>
> Key: CALCITE-6122
> URL: https://issues.apache.org/jira/browse/CALCITE-6122
> Project: Calcite
> Issue Type: Bug
> Components: core
>Affects Versions: 1.36.0
>Reporter: Steve Carlin
>Priority: Major
>
> I hope I'm describing this right.
> I'm coercing an operand in my handmade OperandTypeChecker. Specifically, I'm
> changing a {color:#de350b}SUM(tinyint_col){color} to a
> {color:#de350b}SUM(CAST(tinyint_col as BIGINT)){color} because my database
> can only handle a bigint operand.
> The eventual logical plan does not keep the CAST operand.
> I had this problem in 1.34.0. I noticed that the code changed quite a bit
> going up to 1.36.0, so I say this to mention that this is not a regression.
> I hacked a fix in my environment, but it's too hacky to commit. To fix the
> problem, I changed to the following code in SqlToRelConverter.convertAgg():
>
> {code:java}
> @@ -3369,7 +3372,11 @@ protected void convertAgg(Blackboard bb, SqlSelect
> select,
> final AggConverter aggConverter =
> AggConverter.create(bb,
> (AggregatingSelectScope) validator().getSelectScope(select));
> - createAggImpl(bb, aggConverter, selectList, groupList, having,
> + selectList.accept(aggConverter);
> + final AggConverter aggConverter2 =
> + AggConverter.create(bb,
> + (AggregatingSelectScope) validator().getSelectScope(select));
> + createAggImpl(bb, aggConverter2, selectList, groupList, having,
> orderExprList);
> }
> {code}
> Note that I had the selectList go through the aggConverter visitor. After
> this, the selectList contains the coerced operands. If the aggConverter is
> created based on this new selectList, it will contain the proper information
> in the aggConverter.convertedInputExprs list
> (One other note: There is an assertion in createAggImpl that i had to disable
> in order to get this hack to work where it checks that bb.agg == null)
> I can probably work on this, but I'm not sure how to create a proper test for
> it, as I've never committed anything to Calcite before.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Created] (CALCITE-6122) In SqlToRelConverter, AggConverter doesn't use coerced SqlNodes
Steve Carlin created CALCITE-6122:
-
Summary: In SqlToRelConverter, AggConverter doesn't use coerced
SqlNodes
Key: CALCITE-6122
URL: https://issues.apache.org/jira/browse/CALCITE-6122
Project: Calcite
Issue Type: Bug
Components: core
Affects Versions: 1.36.0
Reporter: Steve Carlin
I hope I'm describing this right.
I'm coercing an operand in my handmade OperandTypeChecker. Specifically, I'm
changing a {color:#de350b}SUM(tinyint_col){color} to a
{color:#de350b}SUM(CAST(tinyint_col as BIGINT)){color} because my database can
only handle a bigint operand.
The eventual logical plan does not keep the CAST operand.
I had this problem in 1.34.0. I noticed that the code changed quite a bit
going up to 1.36.0, so I say this to mention that this is not a regression.
I hacked a fix in my environment, but it's too hacky to commit. To fix the
problem, I changed to the following code in SqlToRelConverter.convertAgg():
{code:java}
@@ -3369,7 +3372,11 @@ protected void convertAgg(Blackboard bb, SqlSelect
select,
final AggConverter aggConverter =
AggConverter.create(bb,
(AggregatingSelectScope) validator().getSelectScope(select));
- createAggImpl(bb, aggConverter, selectList, groupList, having,
+ selectList.accept(aggConverter);
+ final AggConverter aggConverter2 =
+ AggConverter.create(bb,
+ (AggregatingSelectScope) validator().getSelectScope(select));
+ createAggImpl(bb, aggConverter2, selectList, groupList, having,
orderExprList);
}
{code}
Note that I had the selectList go through the aggConverter visitor. After
this, the selectList contains the coerced operands. If the aggConverter is
created based on this new selectList, it will contain the proper information in
the aggConverter.convertedInputExprs list
(One other note: There is an assertion in createAggImpl that i had to disable
in order to get this hack to work where it checks that bb.agg == null)
I can probably work on this, but I'm not sure how to create a proper test for
it, as I've never committed anything to Calcite before.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (CALCITE-5985) FilterTableFunctionTransposeRule should not use "Logical" RelNodes
[ https://issues.apache.org/jira/browse/CALCITE-5985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17763475#comment-17763475 ] Steve Carlin commented on CALCITE-5985: --- Sorry, unfortunately, I do not. I only noticed this because I tried to implement this rule in my own project. > FilterTableFunctionTransposeRule should not use "Logical" RelNodes > -- > > Key: CALCITE-5985 > URL: https://issues.apache.org/jira/browse/CALCITE-5985 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Steve Carlin >Priority: Major > > The FilterTableFunctionTransposeRule uses LogicalTableFunctionScan and > LogicalFilter RelNodes in its rule. We should be using the appropriate > TableFunctionScan and Filter interface RelNodes instead. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Created] (CALCITE-5985) FilterTableFunctionTransposeRule should not use "Logical" RelNodes
Steve Carlin created CALCITE-5985: - Summary: FilterTableFunctionTransposeRule should not use "Logical" RelNodes Key: CALCITE-5985 URL: https://issues.apache.org/jira/browse/CALCITE-5985 Project: Calcite Issue Type: Bug Components: core Reporter: Steve Carlin The FilterTableFunctionTransposeRule uses LogicalTableFunctionScan and LogicalFilter RelNodes in its rule. We should be using the appropriate TableFunctionScan and Filter interface RelNodes instead. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (CALCITE-5425) Should not pushdown filter through aggregate with an empty groupset
[ https://issues.apache.org/jira/browse/CALCITE-5425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17645073#comment-17645073 ] Steve Carlin commented on CALCITE-5425: --- [~FrankZou] Sounds good! > Should not pushdown filter through aggregate with an empty groupset > --- > > Key: CALCITE-5425 > URL: https://issues.apache.org/jira/browse/CALCITE-5425 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Steve Carlin >Priority: Major > > If we are given a query > SELECT count(x) FROM tbl HAVING false; > This query should produce an empty set. > We should not allow a filter to pass through this aggregate. When the > aggregate has no group by, there is always one implied group (for the whole > dataset). So if we apply the filter before the aggregate, the implied group > will still be created. This is not what we want, since the filter should > produce an empty set. -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Created] (CALCITE-5425) Should not pushdown filter through aggregate with an empty groupset
Steve Carlin created CALCITE-5425: - Summary: Should not pushdown filter through aggregate with an empty groupset Key: CALCITE-5425 URL: https://issues.apache.org/jira/browse/CALCITE-5425 Project: Calcite Issue Type: Bug Components: core Reporter: Steve Carlin If we are given a query SELECT count(x) FROM tbl HAVING false; This query should produce an empty set. We should not allow a filter to pass through this aggregate. When the aggregate has no group by, there is always one implied group (for the whole dataset). So if we apply the filter before the aggregate, the implied group will still be created. This is not what we want, since the filter should produce an empty set. -- This message was sent by Atlassian Jira (v8.20.10#820010)
