[jira] [Commented] (CALCITE-1436) AggregateNode NPE for aggregators other than SUM/COUNT
[ https://issues.apache.org/jira/browse/CALCITE-1436?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16513708#comment-16513708 ] Michael Mior commented on CALCITE-1436: --- Sounds good. I added some basic tests in [834ad98e7|https://github.com/apache/calcite/commit/834ad98e714f6df435b171cd898fb70f9bdb0b04]. > AggregateNode NPE for aggregators other than SUM/COUNT > -- > > Key: CALCITE-1436 > URL: https://issues.apache.org/jira/browse/CALCITE-1436 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Gian Merlino >Assignee: Michael Mior >Priority: Major > Fix For: 1.17.0 > > > AggregateNode.getAccumulator does this for any aggregation other than COUNT > or SUM: > final AggImpState agg = new AggImpState(0, call, false); > int stateSize = agg.state.size(); > This NPEs because "state" is null on freshly created AggImpState instances. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-1436) AggregateNode NPE for aggregators other than SUM/COUNT
[ https://issues.apache.org/jira/browse/CALCITE-1436?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16513475#comment-16513475 ] Julian Hyde commented on CALCITE-1436: -- There’s a table called allTypes. I don’t know whether it is usable. As long as there is one test for each age function, regardless of type, I’d be happy. > AggregateNode NPE for aggregators other than SUM/COUNT > -- > > Key: CALCITE-1436 > URL: https://issues.apache.org/jira/browse/CALCITE-1436 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Gian Merlino >Assignee: Michael Mior >Priority: Major > Fix For: 1.17.0 > > > AggregateNode.getAccumulator does this for any aggregation other than COUNT > or SUM: > final AggImpState agg = new AggImpState(0, call, false); > int stateSize = agg.state.size(); > This NPEs because "state" is null on freshly created AggImpState instances. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-1436) AggregateNode NPE for aggregators other than SUM/COUNT
[ https://issues.apache.org/jira/browse/CALCITE-1436?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16513305#comment-16513305 ] Michael Mior commented on CALCITE-1436: --- Any thoughts on the best way to test with different types? I have tests on the Beatles table which only has integer and varchar columns. I assume the easiest approach is to add a couple extra columns? > AggregateNode NPE for aggregators other than SUM/COUNT > -- > > Key: CALCITE-1436 > URL: https://issues.apache.org/jira/browse/CALCITE-1436 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Gian Merlino >Assignee: Michael Mior >Priority: Major > Fix For: 1.17.0 > > > AggregateNode.getAccumulator does this for any aggregation other than COUNT > or SUM: > final AggImpState agg = new AggImpState(0, call, false); > int stateSize = agg.state.size(); > This NPEs because "state" is null on freshly created AggImpState instances. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-1436) AggregateNode NPE for aggregators other than SUM/COUNT
[ https://issues.apache.org/jira/browse/CALCITE-1436?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16512375#comment-16512375 ] Michael Mior commented on CALCITE-1436: --- [~julianhyde] Thanks for keeping tabs on this. I should know better by know on both those points. Sorry for that. Agreed that tests should be added. Fixed in [f84a3eb|https://github.com/apache/calcite/commit/f84a3eb330d4ac69dd412498398909a690e9ce2d] > AggregateNode NPE for aggregators other than SUM/COUNT > -- > > Key: CALCITE-1436 > URL: https://issues.apache.org/jira/browse/CALCITE-1436 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Gian Merlino >Assignee: Michael Mior >Priority: Major > Fix For: 1.17.0 > > > AggregateNode.getAccumulator does this for any aggregation other than COUNT > or SUM: > final AggImpState agg = new AggImpState(0, call, false); > int stateSize = agg.state.size(); > This NPEs because "state" is null on freshly created AggImpState instances. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-1436) AggregateNode NPE for aggregators other than SUM/COUNT
[ https://issues.apache.org/jira/browse/CALCITE-1436?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16511811#comment-16511811 ] Julian Hyde commented on CALCITE-1436: -- [~michaelmior], When you mark resolved please add commit URL and version. [~mgelbana], Can you please add tests? I was able to reproduce easily by extending InterpreterTest.testAggregate(). > AggregateNode NPE for aggregators other than SUM/COUNT > -- > > Key: CALCITE-1436 > URL: https://issues.apache.org/jira/browse/CALCITE-1436 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Gian Merlino >Assignee: Michael Mior >Priority: Major > > AggregateNode.getAccumulator does this for any aggregation other than COUNT > or SUM: > final AggImpState agg = new AggImpState(0, call, false); > int stateSize = agg.state.size(); > This NPEs because "state" is null on freshly created AggImpState instances. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-1436) AggregateNode NPE for aggregators other than SUM/COUNT
[ https://issues.apache.org/jira/browse/CALCITE-1436?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16500452#comment-16500452 ] Muhammad Gelbana commented on CALCITE-1436: --- All tests ran find. I created a Pull request: [https://github.com/apache/calcite/pull/719] > AggregateNode NPE for aggregators other than SUM/COUNT > -- > > Key: CALCITE-1436 > URL: https://issues.apache.org/jira/browse/CALCITE-1436 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Gian Merlino >Assignee: Julian Hyde >Priority: Major > > AggregateNode.getAccumulator does this for any aggregation other than COUNT > or SUM: > final AggImpState agg = new AggImpState(0, call, false); > int stateSize = agg.state.size(); > This NPEs because "state" is null on freshly created AggImpState instances. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-1436) AggregateNode NPE for aggregators other than SUM/COUNT
[ https://issues.apache.org/jira/browse/CALCITE-1436?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16500330#comment-16500330 ] Muhammad Gelbana commented on CALCITE-1436: --- Has anyone created a test case for this ? I'm failing to write a test case that reproduces it. > AggregateNode NPE for aggregators other than SUM/COUNT > -- > > Key: CALCITE-1436 > URL: https://issues.apache.org/jira/browse/CALCITE-1436 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Gian Merlino >Assignee: Julian Hyde >Priority: Major > > AggregateNode.getAccumulator does this for any aggregation other than COUNT > or SUM: > final AggImpState agg = new AggImpState(0, call, false); > int stateSize = agg.state.size(); > This NPEs because "state" is null on freshly created AggImpState instances. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-1436) AggregateNode NPE for aggregators other than SUM/COUNT
[
https://issues.apache.org/jira/browse/CALCITE-1436?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16497927#comment-16497927
]
Muhammad Gelbana commented on CALCITE-1436:
---
Would someone please shed some light on what needs to be done here ? I believe
[~vladimirsitnikov]\[~vlsi] is the author of [this
change|[https://github.com/apache/calcite/commit/ddad58826121d29362539f240ea32660811dc09b#diff-964b02bd21a99d18828586e07bb88aa7].]
Would you please provide more information ?
I can think of two ways to fix this. I either create an accumulator following
the same interface as IntSum, LongSum and DoubleSum and
{code:java}
return new UdaAccumulatorFactory(AggregateFunctionImpl.create(IntMin.class),
call, true); // That's for Integral input{code}
Just as what the SUM branch does.
Or,
Considering that I'm trying to support MIN and MAX aggregate functions, I'll
have to initialize the *state* list with only one element that represents the
calculated minimum\maximum value. Should this expression be of type
ParameterExpression ? This will have to be done in the AggImpState constructor
of course. But this is still complicated to me, I appreciate some
clarifications please.
> AggregateNode NPE for aggregators other than SUM/COUNT
> --
>
> Key: CALCITE-1436
> URL: https://issues.apache.org/jira/browse/CALCITE-1436
> Project: Calcite
> Issue Type: Bug
> Components: core
>Reporter: Gian Merlino
>Assignee: Julian Hyde
>Priority: Major
>
> AggregateNode.getAccumulator does this for any aggregation other than COUNT
> or SUM:
> final AggImpState agg = new AggImpState(0, call, false);
> int stateSize = agg.state.size();
> This NPEs because "state" is null on freshly created AggImpState instances.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-1436) AggregateNode NPE for aggregators other than SUM/COUNT
[ https://issues.apache.org/jira/browse/CALCITE-1436?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16236691#comment-16236691 ] Julian Hyde commented on CALCITE-1436: -- Enumerable uses the volcano execution model i.e. one active thread per query; bindable is capable of using one thread per operator, more suitable for streaming queries. > AggregateNode NPE for aggregators other than SUM/COUNT > -- > > Key: CALCITE-1436 > URL: https://issues.apache.org/jira/browse/CALCITE-1436 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Gian Merlino >Assignee: Julian Hyde >Priority: Major > > AggregateNode.getAccumulator does this for any aggregation other than COUNT > or SUM: > final AggImpState agg = new AggImpState(0, call, false); > int stateSize = agg.state.size(); > This NPEs because "state" is null on freshly created AggImpState instances. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (CALCITE-1436) AggregateNode NPE for aggregators other than SUM/COUNT
[ https://issues.apache.org/jira/browse/CALCITE-1436?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16236679#comment-16236679 ] JD Zheng commented on CALCITE-1436: --- I'd love to contribute. Given the nature of this bug (a member of a class is accessed before it is initialized), it seems that the bindable code path is very immature. I am not sure if I should fix the exact bug or rather make the druid adapter use the enumerable convention. It'll be very helpful if someone can explain me the difference between bindable convention and enumerable convention and why we have both. > AggregateNode NPE for aggregators other than SUM/COUNT > -- > > Key: CALCITE-1436 > URL: https://issues.apache.org/jira/browse/CALCITE-1436 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Gian Merlino >Assignee: Julian Hyde >Priority: Major > > AggregateNode.getAccumulator does this for any aggregation other than COUNT > or SUM: > final AggImpState agg = new AggImpState(0, call, false); > int stateSize = agg.state.size(); > This NPEs because "state" is null on freshly created AggImpState instances. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (CALCITE-1436) AggregateNode NPE for aggregators other than SUM/COUNT
[ https://issues.apache.org/jira/browse/CALCITE-1436?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16236229#comment-16236229 ] Julian Hyde commented on CALCITE-1436: -- Nothing has changed as far as I know. Contributions welcome. > AggregateNode NPE for aggregators other than SUM/COUNT > -- > > Key: CALCITE-1436 > URL: https://issues.apache.org/jira/browse/CALCITE-1436 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Gian Merlino >Assignee: Julian Hyde >Priority: Major > > AggregateNode.getAccumulator does this for any aggregation other than COUNT > or SUM: > final AggImpState agg = new AggImpState(0, call, false); > int stateSize = agg.state.size(); > This NPEs because "state" is null on freshly created AggImpState instances. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (CALCITE-1436) AggregateNode NPE for aggregators other than SUM/COUNT
[ https://issues.apache.org/jira/browse/CALCITE-1436?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16234944#comment-16234944 ] JD Zheng commented on CALCITE-1436: --- Any update on this bug? We are hitting this bug while using the druid-adapter. I notice that the EnumerableAggregate works just fine. But the druid-adapter will generate a physical plan to use BindableAggregate instead of EnumberableAggregate. Is there any quick fix to make the druid-adapter to use the EnumberableAggregate instead of BindableAggregate? > AggregateNode NPE for aggregators other than SUM/COUNT > -- > > Key: CALCITE-1436 > URL: https://issues.apache.org/jira/browse/CALCITE-1436 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Gian Merlino >Assignee: Julian Hyde >Priority: Major > > AggregateNode.getAccumulator does this for any aggregation other than COUNT > or SUM: > final AggImpState agg = new AggImpState(0, call, false); > int stateSize = agg.state.size(); > This NPEs because "state" is null on freshly created AggImpState instances. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (CALCITE-1436) AggregateNode NPE for aggregators other than SUM/COUNT
[ https://issues.apache.org/jira/browse/CALCITE-1436?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15573182#comment-15573182 ] Gian Merlino commented on CALCITE-1436: --- Yep, you're right, this and CALCITE-1404 were both things I ran into while looking into Druid SQL stuff. Specifically there are two areas that the interpreter seemed useful: 1) unit tests of the Druid pushdown stuff; I wanted to be able to do a cross-engine test with Druid vs some other engine handling the same SQL. I was looking at Calcite's interpreter as the "some other engine". 2) in production, handling of bits of the SQL query that can't be pushed down to Druid. I'm still not sure if this is a good idea or not, maybe it's better to just fail fast if we can't push down 100% of the query. Trying to "fill in the gaps" can surprise users by executing a plan that is so inefficient that it's doomed to fail. But if we end up wanting to do this regardless of that risk, then Calcite's interpreter seems useful for that. > AggregateNode NPE for aggregators other than SUM/COUNT > -- > > Key: CALCITE-1436 > URL: https://issues.apache.org/jira/browse/CALCITE-1436 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Gian Merlino >Assignee: Julian Hyde > > AggregateNode.getAccumulator does this for any aggregation other than COUNT > or SUM: > final AggImpState agg = new AggImpState(0, call, false); > int stateSize = agg.state.size(); > This NPEs because "state" is null on freshly created AggImpState instances. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CALCITE-1436) AggregateNode NPE for aggregators other than SUM/COUNT
[ https://issues.apache.org/jira/browse/CALCITE-1436?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15573071#comment-15573071 ] Julian Hyde commented on CALCITE-1436: -- I'm not surprised by this. AggregateNode has not been extensively tested. I surmise that you're looking at various options for Druid-SQL. We should discuss. EnumerableAggregate is more thoroughly tested, and more efficient (since it is compiled, not interpreted). AggregateNode (and the other interpreter operators) could potentially be much more efficient than EnumerableAggregate, if we made them take a batch of rows (say in Arrow format). But by that point we would have an execution engine similar to Drill. Pulling the engine out of Drill should also be considered. > AggregateNode NPE for aggregators other than SUM/COUNT > -- > > Key: CALCITE-1436 > URL: https://issues.apache.org/jira/browse/CALCITE-1436 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Gian Merlino >Assignee: Julian Hyde > > AggregateNode.getAccumulator does this for any aggregation other than COUNT > or SUM: > final AggImpState agg = new AggImpState(0, call, false); > int stateSize = agg.state.size(); > This NPEs because "state" is null on freshly created AggImpState instances. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
