[jira] [Commented] (CALCITE-4191) Improve the logic of creating aggregate calls
[
https://issues.apache.org/jira/browse/CALCITE-4191?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17185096#comment-17185096
]
Liya Fan commented on CALCITE-4191:
---
[~Chunwei Lei] Thanks for your feedback.
It seems the indirect way always generates the AggregateCall along with the
Aggregate rel node. For scenarios where we only need the AggregateCall objects
(e.g. Window#getAggregateCalls), it is more convenient to use the
Aggregate#create methods.
> Improve the logic of creating aggregate calls
> -
>
> Key: CALCITE-4191
> URL: https://issues.apache.org/jira/browse/CALCITE-4191
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> According to the current code base, the only way to create \{{AggregateCall}}
> objects is by calling one of the two \{{AggregateCall#create}} methods (other
> create methods are deprecated).
> The two {{create}} methods have 9 and 11 parameters, respectively, 3 of which
> are booleans and 2 are ints. We find this makes the code less readable and
> error-prone, as some bugs are caused by specifying the wrong parameters.
> In this issue, we improve the related logic by the builder pattern, which
> results in the following benefits:
> 1. By creating the objects by the builder pattern, there is no need to
> maintain multiple overrides of the {{create}} methods.
> 2. There is no need to maintain multiple overrides of the {{copy}} methods,
> either.
> 3. The code becomes more readable and less error-prone, as it is less like to
> specify the wrong parameter.
> 4. Creating {{AggregateCall}} objects becomes easier, as the user does not
> have specify the default parameters repeatedly.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-4191) Improve the logic of creating aggregate calls
[
https://issues.apache.org/jira/browse/CALCITE-4191?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17184958#comment-17184958
]
Julian Hyde commented on CALCITE-4191:
--
[~Chunwei Lei], There is already a way - but it's indirect. Use a
{{RelBuilder}}. As an example of code that does this, see
[AggregateExtractProjectRule|https://github.com/apache/calcite/blob/885a3da76cfd59171624dc569e83e93a5bdaffe3/core/src/main/java/org/apache/calcite/rel/rules/AggregateExtractProjectRule.java#L126]:
{code:java}
final List newAggCallList = new ArrayList<>();
for (AggregateCall aggCall : aggregate.getAggCallList()) {
final ImmutableList args =
relBuilder.fields(
Mappings.apply2(mapping, aggCall.getArgList()));
final RexNode filterArg = aggCall.filterArg < 0 ? null
: relBuilder.field(Mappings.apply(mapping, aggCall.filterArg));
newAggCallList.add(
relBuilder.aggregateCall(aggCall.getAggregation(), args)
.distinct(aggCall.isDistinct())
.filter(filterArg)
.approximate(aggCall.isApproximate())
.sort(relBuilder.fields(aggCall.collation))
.as(aggCall.name));
}
final RelBuilder.GroupKey groupKey =
relBuilder.groupKey(newGroupSet, newGroupSets);
relBuilder.aggregate(groupKey, newAggCallList);
call.transformTo(relBuilder.build());
{code}
Those {{RelBuilder.AggCall}} objects are basically builders.
{{relBuilder.aggregate}} converts them into {{AggregateCall}} instances.
> Improve the logic of creating aggregate calls
> -
>
> Key: CALCITE-4191
> URL: https://issues.apache.org/jira/browse/CALCITE-4191
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> According to the current code base, the only way to create \{{AggregateCall}}
> objects is by calling one of the two \{{AggregateCall#create}} methods (other
> create methods are deprecated).
> The two {{create}} methods have 9 and 11 parameters, respectively, 3 of which
> are booleans and 2 are ints. We find this makes the code less readable and
> error-prone, as some bugs are caused by specifying the wrong parameters.
> In this issue, we improve the related logic by the builder pattern, which
> results in the following benefits:
> 1. By creating the objects by the builder pattern, there is no need to
> maintain multiple overrides of the {{create}} methods.
> 2. There is no need to maintain multiple overrides of the {{copy}} methods,
> either.
> 3. The code becomes more readable and less error-prone, as it is less like to
> specify the wrong parameter.
> 4. Creating {{AggregateCall}} objects becomes easier, as the user does not
> have specify the default parameters repeatedly.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-4191) Improve the logic of creating aggregate calls
[
https://issues.apache.org/jira/browse/CALCITE-4191?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17184884#comment-17184884
]
Chunwei Lei commented on CALCITE-4191:
--
Personally I would like to have such a way to create {{AggregateCall}}
considering so many parameters.
> Improve the logic of creating aggregate calls
> -
>
> Key: CALCITE-4191
> URL: https://issues.apache.org/jira/browse/CALCITE-4191
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> According to the current code base, the only way to create \{{AggregateCall}}
> objects is by calling one of the two \{{AggregateCall#create}} methods (other
> create methods are deprecated).
> The two {{create}} methods have 9 and 11 parameters, respectively, 3 of which
> are booleans and 2 are ints. We find this makes the code less readable and
> error-prone, as some bugs are caused by specifying the wrong parameters.
> In this issue, we improve the related logic by the builder pattern, which
> results in the following benefits:
> 1. By creating the objects by the builder pattern, there is no need to
> maintain multiple overrides of the {{create}} methods.
> 2. There is no need to maintain multiple overrides of the {{copy}} methods,
> either.
> 3. The code becomes more readable and less error-prone, as it is less like to
> specify the wrong parameter.
> 4. Creating {{AggregateCall}} objects becomes easier, as the user does not
> have specify the default parameters repeatedly.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-4191) Improve the logic of creating aggregate calls
[
https://issues.apache.org/jira/browse/CALCITE-4191?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17183868#comment-17183868
]
Liya Fan commented on CALCITE-4191:
---
Sure. I see.
> Improve the logic of creating aggregate calls
> -
>
> Key: CALCITE-4191
> URL: https://issues.apache.org/jira/browse/CALCITE-4191
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> According to the current code base, the only way to create \{{AggregateCall}}
> objects is by calling one of the two \{{AggregateCall#create}} methods (other
> create methods are deprecated).
> The two {{create}} methods have 9 and 11 parameters, respectively, 3 of which
> are booleans and 2 are ints. We find this makes the code less readable and
> error-prone, as some bugs are caused by specifying the wrong parameters.
> In this issue, we improve the related logic by the builder pattern, which
> results in the following benefits:
> 1. By creating the objects by the builder pattern, there is no need to
> maintain multiple overrides of the {{create}} methods.
> 2. There is no need to maintain multiple overrides of the {{copy}} methods,
> either.
> 3. The code becomes more readable and less error-prone, as it is less like to
> specify the wrong parameter.
> 4. Creating {{AggregateCall}} objects becomes easier, as the user does not
> have specify the default parameters repeatedly.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-4191) Improve the logic of creating aggregate calls
[
https://issues.apache.org/jira/browse/CALCITE-4191?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17183814#comment-17183814
]
Julian Hyde commented on CALCITE-4191:
--
Who is 'we'? It sounds as if you have been having design discussions with
colleagues. If so, we need to talk. In the Apache Way, off-list discussions and
decisions are discouraged. That's why I suggest creating a JIRA case (or
starting a dev thread) before working on code.
> Improve the logic of creating aggregate calls
> -
>
> Key: CALCITE-4191
> URL: https://issues.apache.org/jira/browse/CALCITE-4191
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> According to the current code base, the only way to create \{{AggregateCall}}
> objects is by calling one of the two \{{AggregateCall#create}} methods (other
> create methods are deprecated).
> The two {{create}} methods have 9 and 11 parameters, respectively, 3 of which
> are booleans and 2 are ints. We find this makes the code less readable and
> error-prone, as some bugs are caused by specifying the wrong parameters.
> In this issue, we improve the related logic by the builder pattern, which
> results in the following benefits:
> 1. By creating the objects by the builder pattern, there is no need to
> maintain multiple overrides of the {{create}} methods.
> 2. There is no need to maintain multiple overrides of the {{copy}} methods,
> either.
> 3. The code becomes more readable and less error-prone, as it is less like to
> specify the wrong parameter.
> 4. Creating {{AggregateCall}} objects becomes easier, as the user does not
> have specify the default parameters repeatedly.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-4191) Improve the logic of creating aggregate calls
[
https://issues.apache.org/jira/browse/CALCITE-4191?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17183808#comment-17183808
]
Liya Fan commented on CALCITE-4191:
---
Thanks for your kind reminder. My example above is not a good one.
I have to admit that we did not explore the idea of using RelBuilder very much,
because it seems the {{AggregateCall#create}} methods are more convenient (at
least for our scenarios). (RelBuilder only indirectly produces AggregateCall
objects, which are transformed from AggCallImpl objects)
We also find that in the code base, the {{AggregateCall#create}} methods are
widely used. So I think they are the better option, at least for the scenarios
concerned.
> Improve the logic of creating aggregate calls
> -
>
> Key: CALCITE-4191
> URL: https://issues.apache.org/jira/browse/CALCITE-4191
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> According to the current code base, the only way to create \{{AggregateCall}}
> objects is by calling one of the two \{{AggregateCall#create}} methods (other
> create methods are deprecated).
> The two {{create}} methods have 9 and 11 parameters, respectively, 3 of which
> are booleans and 2 are ints. We find this makes the code less readable and
> error-prone, as some bugs are caused by specifying the wrong parameters.
> In this issue, we improve the related logic by the builder pattern, which
> results in the following benefits:
> 1. By creating the objects by the builder pattern, there is no need to
> maintain multiple overrides of the {{create}} methods.
> 2. There is no need to maintain multiple overrides of the {{copy}} methods,
> either.
> 3. The code becomes more readable and less error-prone, as it is less like to
> specify the wrong parameter.
> 4. Creating {{AggregateCall}} objects becomes easier, as the user does not
> have specify the default parameters repeatedly.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-4191) Improve the logic of creating aggregate calls
[
https://issues.apache.org/jira/browse/CALCITE-4191?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17183780#comment-17183780
]
Julian Hyde commented on CALCITE-4191:
--
Are you are of {{RelBuilder.AggCall.approximate(boolean)}}?
> Improve the logic of creating aggregate calls
> -
>
> Key: CALCITE-4191
> URL: https://issues.apache.org/jira/browse/CALCITE-4191
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> According to the current code base, the only way to create \{{AggregateCall}}
> objects is by calling one of the two \{{AggregateCall#create}} methods (other
> create methods are deprecated).
> The two {{create}} methods have 9 and 11 parameters, respectively, 3 of which
> are booleans and 2 are ints. We find this makes the code less readable and
> error-prone, as some bugs are caused by specifying the wrong parameters.
> In this issue, we improve the related logic by the builder pattern, which
> results in the following benefits:
> 1. By creating the objects by the builder pattern, there is no need to
> maintain multiple overrides of the {{create}} methods.
> 2. There is no need to maintain multiple overrides of the {{copy}} methods,
> either.
> 3. The code becomes more readable and less error-prone, as it is less like to
> specify the wrong parameter.
> 4. Creating {{AggregateCall}} objects becomes easier, as the user does not
> have specify the default parameters repeatedly.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-4191) Improve the logic of creating aggregate calls
[
https://issues.apache.org/jira/browse/CALCITE-4191?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17183776#comment-17183776
]
Liya Fan commented on CALCITE-4191:
---
[~julianhyde] Thanks for your kind reminder. Sorry I forgot to open a JIRA when
starting the work. I need to be mindful next time.
IMHO, RelBuildre solves the problem only when it is applicable. For other
scenarios, we have to fall back on the {{AggregateCall#create}} methods. For
example, all the public APIs of RelBuilder (that are not not deprecated) for
AggregateCall can only be applied when distinct = false, approximate = false
and ignoreNull = false.
> Improve the logic of creating aggregate calls
> -
>
> Key: CALCITE-4191
> URL: https://issues.apache.org/jira/browse/CALCITE-4191
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> According to the current code base, the only way to create \{{AggregateCall}}
> objects is by calling one of the two \{{AggregateCall#create}} methods (other
> create methods are deprecated).
> The two {{create}} methods have 9 and 11 parameters, respectively, 3 of which
> are booleans and 2 are ints. We find this makes the code less readable and
> error-prone, as some bugs are caused by specifying the wrong parameters.
> In this issue, we improve the related logic by the builder pattern, which
> results in the following benefits:
> 1. By creating the objects by the builder pattern, there is no need to
> maintain multiple overrides of the {{create}} methods.
> 2. There is no need to maintain multiple overrides of the {{copy}} methods,
> either.
> 3. The code becomes more readable and less error-prone, as it is less like to
> specify the wrong parameter.
> 4. Creating {{AggregateCall}} objects becomes easier, as the user does not
> have specify the default parameters repeatedly.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-4191) Improve the logic of creating aggregate calls
[
https://issues.apache.org/jira/browse/CALCITE-4191?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17183742#comment-17183742
]
Julian Hyde commented on CALCITE-4191:
--
Did you consider using RelBuilder? I think it solves this problem.
I wish you would log a JIRA case when you start work, rather than creating a
JIRA case and PR at the same time.
> Improve the logic of creating aggregate calls
> -
>
> Key: CALCITE-4191
> URL: https://issues.apache.org/jira/browse/CALCITE-4191
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Liya Fan
>Assignee: Liya Fan
>Priority: Major
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> According to the current code base, the only way to create \{{AggregateCall}}
> objects is by calling one of the two \{{AggregateCall#create}} methods (other
> create methods are deprecated).
> The two {{create}} methods have 9 and 11 parameters, respectively, 3 of which
> are booleans and 2 are ints. We find this makes the code less readable and
> error-prone, as some bugs are caused by specifying the wrong parameters.
> In this issue, we improve the related logic by the builder pattern, which
> results in the following benefits:
> 1. By creating the objects by the builder pattern, there is no need to
> maintain multiple overrides of the {{create}} methods.
> 2. There is no need to maintain multiple overrides of the {{copy}} methods,
> either.
> 3. The code becomes more readable and less error-prone, as it is less like to
> specify the wrong parameter.
> 4. Creating {{AggregateCall}} objects becomes easier, as the user does not
> have specify the default parameters repeatedly.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
