[jira] [Commented] (CALCITE-3085) Unused stack field in RelShuttleImpl
[
https://issues.apache.org/jira/browse/CALCITE-3085?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16851082#comment-16851082
]
Julian Hyde commented on CALCITE-3085:
--
How about creating a class RelBasicShuttle implements RelShuttle, and (if you
wish) class RelShuttleImpl extends RelBasicShuttle. Change existing classes to
extend RelBasicSthuttle rather than RelShuttleImpl. Basically the same as your
suggestion but without the rename.
> Unused stack field in RelShuttleImpl
>
>
> Key: CALCITE-3085
> URL: https://issues.apache.org/jira/browse/CALCITE-3085
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Laurent Goujon
>Assignee: Laurent Goujon
>Priority: Minor
> Labels: pull-request-available
> Time Spent: 40m
> Remaining Estimate: 0h
>
> {{RelShuttleImpl}} class has a protected {{stack}} field which is being
> populated when going over children, but those content is actually never used.
> In Calcite code base, no subclasses are actually using the content of the
> stack ({{CorelMapBuilder}} is populating the stack but does not read the
> content back either).
> Searching code on github didn't show any usage of it either (but this is not
> foolprof).
> As maintaining this stack has a non-negligible impact on memory/performance,
> I would suggest to remove the field.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-3085) Unused stack field in RelShuttleImpl
[
https://issues.apache.org/jira/browse/CALCITE-3085?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16850155#comment-16850155
]
Laurent Goujon commented on CALCITE-3085:
-
It's so useful no subclasses uses it (unlike modCound) :) TBH I don't have the
observation anymore as it was a change one of my team mate made some time
(year) ago, and didn't left much detail except for avoiding the extra
allocation, and also being able to create threadsafe instances of
RelShuttleImpl (and so being able to use singleton).
I also offered a second alternative (see my comment above) to create an
alternative implementation. If people are more comfortable with that second
approach:
- Would people be okay of changing current calcite shuttles to use the
alternative? And replacing instances of RelShuttleImpl with no state by
singleton instances?
- Would people be okay with renaming current RelShuttleImpl
StackingRelShuttleImpl and the alternative RelShuttleImpl
> Unused stack field in RelShuttleImpl
>
>
> Key: CALCITE-3085
> URL: https://issues.apache.org/jira/browse/CALCITE-3085
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Laurent Goujon
>Assignee: Laurent Goujon
>Priority: Minor
> Labels: pull-request-available
> Time Spent: 40m
> Remaining Estimate: 0h
>
> {{RelShuttleImpl}} class has a protected {{stack}} field which is being
> populated when going over children, but those content is actually never used.
> In Calcite code base, no subclasses are actually using the content of the
> stack ({{CorelMapBuilder}} is populating the stack but does not read the
> content back either).
> Searching code on github didn't show any usage of it either (but this is not
> foolprof).
> As maintaining this stack has a non-negligible impact on memory/performance,
> I would suggest to remove the field.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-3085) Unused stack field in RelShuttleImpl
[
https://issues.apache.org/jira/browse/CALCITE-3085?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16850140#comment-16850140
]
Julian Hyde commented on CALCITE-3085:
--
Has this issue shown up empirically?
The stack is implemented as a {{ArrayDeque}}, so push and pop should be very
few machine instructions.
{{RelShuttleImpl}} is not the only possible implementation of {{RelShuttle}}.
It is just a good general-purpose implementation. If it doesn't meet your
needs, you can create your own.
No sub-classes in Calcite use the {{stack}} field, but it's there, and it is
potentially useful, if I am looking for say a {{Filter}} within a {{Project}}
within a {{Filter}}.
Consider {{AbstractList}} class in the JDK. It contains a {{modCount}} field is
useful for most implementations (prevents concurrent modifications) but is
unnecessary overhead for sub-classes that are immutable. To avoid that
overhead, I created {{AbstractImmutableList}}, but I wouldn't suggest removing
{{modCount}} from {{AbstractList}}.
> Unused stack field in RelShuttleImpl
>
>
> Key: CALCITE-3085
> URL: https://issues.apache.org/jira/browse/CALCITE-3085
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Laurent Goujon
>Assignee: Laurent Goujon
>Priority: Minor
> Labels: pull-request-available
> Time Spent: 40m
> Remaining Estimate: 0h
>
> {{RelShuttleImpl}} class has a protected {{stack}} field which is being
> populated when going over children, but those content is actually never used.
> In Calcite code base, no subclasses are actually using the content of the
> stack ({{CorelMapBuilder}} is populating the stack but does not read the
> content back either).
> Searching code on github didn't show any usage of it either (but this is not
> foolprof).
> As maintaining this stack has a non-negligible impact on memory/performance,
> I would suggest to remove the field.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-3085) Unused stack field in RelShuttleImpl
[
https://issues.apache.org/jira/browse/CALCITE-3085?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16847011#comment-16847011
]
Haisheng Yuan commented on CALCITE-3085:
I can't find any usage of the stack either. So I agree to remove it.
> Unused stack field in RelShuttleImpl
>
>
> Key: CALCITE-3085
> URL: https://issues.apache.org/jira/browse/CALCITE-3085
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Laurent Goujon
>Assignee: Laurent Goujon
>Priority: Minor
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> {{RelShuttleImpl}} class has a protected {{stack}} field which is being
> populated when going over children, but those content is actually never used.
> In Calcite code base, no subclasses are actually using the content of the
> stack ({{CorelMapBuilder}} is populating the stack but does not read the
> content back either).
> Searching code on github didn't show any usage of it either (but this is not
> foolprof).
> As maintaining this stack has a non-negligible impact on memory/performance,
> I would suggest to remove the field.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-3085) Unused stack field in RelShuttleImpl
[
https://issues.apache.org/jira/browse/CALCITE-3085?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16846395#comment-16846395
]
Laurent Goujon commented on CALCITE-3085:
-
This is why I did check on Github. [~danny0405] are you aware of any class
using the stack?
> Unused stack field in RelShuttleImpl
>
>
> Key: CALCITE-3085
> URL: https://issues.apache.org/jira/browse/CALCITE-3085
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Laurent Goujon
>Assignee: Laurent Goujon
>Priority: Minor
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> {{RelShuttleImpl}} class has a protected {{stack}} field which is being
> populated when going over children, but those content is actually never used.
> In Calcite code base, no subclasses are actually using the content of the
> stack ({{CorelMapBuilder}} is populating the stack but does not read the
> content back either).
> Searching code on github didn't show any usage of it either (but this is not
> foolprof).
> As maintaining this stack has a non-negligible impact on memory/performance,
> I would suggest to remove the field.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-3085) Unused stack field in RelShuttleImpl
[
https://issues.apache.org/jira/browse/CALCITE-3085?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16846349#comment-16846349
]
Danny Chan commented on CALCITE-3085:
-
I don't think it's a good idea to remove the stack, this stack provides ability
for child nodes to have access to parent nodes, although this is not that
common but removing this will cause a regression. We can not make sure other
engines with Calcite do not use this stack.
> Unused stack field in RelShuttleImpl
>
>
> Key: CALCITE-3085
> URL: https://issues.apache.org/jira/browse/CALCITE-3085
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Reporter: Laurent Goujon
>Assignee: Laurent Goujon
>Priority: Minor
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> {{RelShuttleImpl}} class has a protected {{stack}} field which is being
> populated when going over children, but those content is actually never used.
> In Calcite code base, no subclasses are actually using the content of the
> stack ({{CorelMapBuilder}} is populating the stack but does not read the
> content back either).
> Searching code on github didn't show any usage of it either (but this is not
> foolprof).
> As maintaining this stack has a non-negligible impact on memory/performance,
> I would suggest to remove the field.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-3085) Unused stack field in RelShuttleImpl
[
https://issues.apache.org/jira/browse/CALCITE-3085?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16846271#comment-16846271
]
Laurent Goujon commented on CALCITE-3085:
-
Alternative would be to create a variant of the class without the stack and
move Calcite classes over, but like I said, didn't find usage of that field
(and it seems we don't have a strong policy of never breaking compatibility,
although we tried to avoid to).
> Unused stack field in RelShuttleImpl
>
>
> Key: CALCITE-3085
> URL: https://issues.apache.org/jira/browse/CALCITE-3085
> Project: Calcite
> Issue Type: Bug
> Components: core
>Reporter: Laurent Goujon
>Assignee: Laurent Goujon
>Priority: Major
>
> {{RelShuttleImpl}} class has a protected {{stack}} field which is being
> populated when going over children, but those content is actually never used.
> In Calcite code base, no subclasses are actually using the content of the
> stack ({{CorelMapBuilder}} is populating the stack but does not read the
> content back either).
> Searching code on github didn't show any usage of it either (but this is not
> foolprof).
> As maintaining this stack has a non-negligible impact on memory/performance,
> I would suggest to remove the field.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
