You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@calcite.apache.org by Vladimir Sitnikov <si...@gmail.com> on 2021/12/13 14:54:55 UTC
RelNode semantic evolution vs adapter implementations
Hi,
It turns out Calcite's nodes can change semantics without notification for
the end-users.
Here are the release notes for Calcite 1.21, and it says **nothing** like
"Ensure you handle Filter#variablesSet in case you implement Filter or in
case you transform LogicalFilter in your rules"
https://calcite.apache.org/news/2019/09/11/release-1.21.0/
On top of that, the in-core adapters fail to handle that properly. For
example, MongoFilter discards Filter#variablesSet.
Can we please stop changing the semantics of RelNodes or can we have a
better way to detect the changes in the client code?
What if we add a "version" property to the corresponding RelNodes, and we
increment it every time the semantic changes?
Then client APIs could be coded like "ok, I'm prepared to handle Project
v4, and Filter v5" (e.g. make "version" required when registering a rule),
and there will be a runtime error in case Calcite generates Filter v6 in
runtime.
---
Sample case:
CALCITE-3183 Trimming method for Filter rel uses wrong traitSet
CALCITE-4913 Correlated variables in a select list are not deduplicated
The case there is that "correlation variables" are added to the Logical*
nodes (e.g. LogicalFilter, LogicalProject).
Unfortunately, that change is hard to notice: there's no compilation
failure, and there's no runtime error.
The old client code just discards "correlation variables", and I guess it
would result in wrong results or something like that.
Silent wrong results is a really sad outcome from the database.
CALCITE-4913 / PR#2623 adds Project#variablesSet property as well, and I
guess it would in the same hidden semantic loss.
Vladimir
Re: RelNode semantic evolution vs adapter implementations
Posted by Vladimir Ozerov <pp...@gmail.com>.
The answer depends on the definition of "wrong". Rules may produce
non-equivalent operators, the validator may accidentally change how type is
deduced for a specific expression, metadata may cause incorrect cost
calculation and pruning of promising plans, leading to a hang in a
downstream application.
Correctness and plan quality of real optimizers depend on all these
components together. Versioning of only one type of the component (e.g.,
RelNode) is likely to be insufficient for an advanced optimizer, as there
are many other sources of regressions. At the same time, some projects may
definitively benefit from even limited versioning. With this in mind, IMO
if we decide to add such versioning, it should be non-intrusive (i.e.,
require little to no efforts to disable it completely), so that the
downstream projects retain enough flexibility on how to manage the
optimizer's quality.
Regards,
Vladimir.
вт, 14 дек. 2021 г. в 12:01, Vladimir Sitnikov <sitnikov.vladimir@gmail.com
>:
> >some direct or indirect changes (metadata, rules, validator, etc)
> >may cause changes to plans around the given RelNode in the downstream
> >project
>
> Do you think changes like "metadata, rules, validator" can cause "wrong
> results" issues?
> What could be the issues caused by "metadata, rules, validator" changes?
>
> I know ObjectWeb ASM bytecode manipulation library uses the concepts of
> versions for quite some time, and it seems to work for them.
>
> As you subclass, say, ClassVisitor, you have to pass the API version which
> your visitor supports.
> Then ASM fails in the runtime if the visitor happens to be applied with a
> too recent bytecode.
>
> >and gets the source code of the test that he includes into his project and
> CI
>
> Source code and bytecode parsing could be an alternative option (for other
> cases), however, it is not clear how to tell
> if the rule in question has been updated to support the new Project/Filter
> contract.
>
> Vladimir
>
Re: RelNode semantic evolution vs adapter implementations
Posted by Vladimir Sitnikov <si...@gmail.com>.
>some direct or indirect changes (metadata, rules, validator, etc)
>may cause changes to plans around the given RelNode in the downstream
>project
Do you think changes like "metadata, rules, validator" can cause "wrong
results" issues?
What could be the issues caused by "metadata, rules, validator" changes?
I know ObjectWeb ASM bytecode manipulation library uses the concepts of
versions for quite some time, and it seems to work for them.
As you subclass, say, ClassVisitor, you have to pass the API version which
your visitor supports.
Then ASM fails in the runtime if the visitor happens to be applied with a
too recent bytecode.
>and gets the source code of the test that he includes into his project and
CI
Source code and bytecode parsing could be an alternative option (for other
cases), however, it is not clear how to tell
if the rule in question has been updated to support the new Project/Filter
contract.
Vladimir
Re: RelNode semantic evolution vs adapter implementations
Posted by Vladimir Ozerov <pp...@gmail.com>.
Practically, Apache Calcite doesn’t have public API. Real projects usually
override not only public extension points, but also internals, such as
SqlValidatorImpl or VolcanoPlanner.
Given the flexibility that Apache Calcite provides, this lack of public API
surface is not necessarily a bad thing, since you may change almost
everything without forking the project. On the other hand, almost any
change to any part of Calcite code base may cause regressions in downstream
projects.
I am not sure there is an ideal way of versioning rels because even if you
do so, some direct or indirect changes (metadata, rules, validator, etc)
may cause changes to plans around the given RelNode in the downstream
project.
Maybe instead of providing versions manually, we may expose some sort of
generated signatures for different components - rels, rules, metadata
handlers, golden plans, etc. Then, we may provide the ability to verify
expected and actual signatures, e.g., using some code generation: user
invokes a command with the list of interesting classes, and gets the source
code of the test that he includes into his project and CI. Now, when the
user migrates to the new version, tests for changed entities will fail, and
user will investigate the associated changes.
The main difference s from the original proposal:
1. Wider scope, because validation of rels is often not sufficient.
2. Automation, because as implement or you cannot always predict the effect
of your changes (e.g., metadata).
WDYT?
Вт, 14 дек. 2021 г. в 01:49, Konstantin Orlov <ko...@gmail.com>:
> > The case there is that "correlation variables" are added to the Logical*
> nodes (e.g. LogicalFilter, LogicalProject).
>
> BTW, "correlation variables" were added to a LogicalFilter in CALCITE-816.
> So what is wrong with CALCITE-3183?
>
> вт, 14 дек. 2021 г. в 01:24, Konstantin Orlov <ko...@gmail.com>:
>
> > Vladimir, could you please clarify in what way the PR#2623 changes
> > the semantics?
> >
> > The correlated project is already possible in the master. The
> MongoProject
> > already discards variablesSet (simply because it's currently not stored
> > for
> > project node) and either fails or returns invalid results. This behavior
> > (alas,
> > incorrect) will be preserved after this patch.
> >
> > пн, 13 дек. 2021 г. в 17:55, Vladimir Sitnikov <
> > sitnikov.vladimir@gmail.com>:
> >
> >> Hi,
> >>
> >> It turns out Calcite's nodes can change semantics without notification
> for
> >> the end-users.
> >>
> >> Here are the release notes for Calcite 1.21, and it says **nothing**
> like
> >> "Ensure you handle Filter#variablesSet in case you implement Filter or
> in
> >> case you transform LogicalFilter in your rules"
> >> https://calcite.apache.org/news/2019/09/11/release-1.21.0/
> >>
> >> On top of that, the in-core adapters fail to handle that properly. For
> >> example, MongoFilter discards Filter#variablesSet.
> >>
> >> Can we please stop changing the semantics of RelNodes or can we have a
> >> better way to detect the changes in the client code?
> >>
> >> What if we add a "version" property to the corresponding RelNodes, and
> we
> >> increment it every time the semantic changes?
> >> Then client APIs could be coded like "ok, I'm prepared to handle Project
> >> v4, and Filter v5" (e.g. make "version" required when registering a
> rule),
> >> and there will be a runtime error in case Calcite generates Filter v6 in
> >> runtime.
> >>
> >> ---
> >>
> >> Sample case:
> >> CALCITE-3183 Trimming method for Filter rel uses wrong traitSet
> >> CALCITE-4913 Correlated variables in a select list are not deduplicated
> >>
> >> The case there is that "correlation variables" are added to the Logical*
> >> nodes (e.g. LogicalFilter, LogicalProject).
> >> Unfortunately, that change is hard to notice: there's no compilation
> >> failure, and there's no runtime error.
> >>
> >> The old client code just discards "correlation variables", and I guess
> it
> >> would result in wrong results or something like that.
> >> Silent wrong results is a really sad outcome from the database.
> >>
> >> CALCITE-4913 / PR#2623 adds Project#variablesSet property as well, and I
> >> guess it would in the same hidden semantic loss.
> >>
> >> Vladimir
> >>
> >
> >
> > --
> > Regards,
> > Konstantin Orlov
> >
>
>
> --
> Regards,
> Konstantin Orlov
>
Re: RelNode semantic evolution vs adapter implementations
Posted by Konstantin Orlov <ko...@gmail.com>.
> The case there is that "correlation variables" are added to the Logical*
nodes (e.g. LogicalFilter, LogicalProject).
BTW, "correlation variables" were added to a LogicalFilter in CALCITE-816.
So what is wrong with CALCITE-3183?
вт, 14 дек. 2021 г. в 01:24, Konstantin Orlov <ko...@gmail.com>:
> Vladimir, could you please clarify in what way the PR#2623 changes
> the semantics?
>
> The correlated project is already possible in the master. The MongoProject
> already discards variablesSet (simply because it's currently not stored
> for
> project node) and either fails or returns invalid results. This behavior
> (alas,
> incorrect) will be preserved after this patch.
>
> пн, 13 дек. 2021 г. в 17:55, Vladimir Sitnikov <
> sitnikov.vladimir@gmail.com>:
>
>> Hi,
>>
>> It turns out Calcite's nodes can change semantics without notification for
>> the end-users.
>>
>> Here are the release notes for Calcite 1.21, and it says **nothing** like
>> "Ensure you handle Filter#variablesSet in case you implement Filter or in
>> case you transform LogicalFilter in your rules"
>> https://calcite.apache.org/news/2019/09/11/release-1.21.0/
>>
>> On top of that, the in-core adapters fail to handle that properly. For
>> example, MongoFilter discards Filter#variablesSet.
>>
>> Can we please stop changing the semantics of RelNodes or can we have a
>> better way to detect the changes in the client code?
>>
>> What if we add a "version" property to the corresponding RelNodes, and we
>> increment it every time the semantic changes?
>> Then client APIs could be coded like "ok, I'm prepared to handle Project
>> v4, and Filter v5" (e.g. make "version" required when registering a rule),
>> and there will be a runtime error in case Calcite generates Filter v6 in
>> runtime.
>>
>> ---
>>
>> Sample case:
>> CALCITE-3183 Trimming method for Filter rel uses wrong traitSet
>> CALCITE-4913 Correlated variables in a select list are not deduplicated
>>
>> The case there is that "correlation variables" are added to the Logical*
>> nodes (e.g. LogicalFilter, LogicalProject).
>> Unfortunately, that change is hard to notice: there's no compilation
>> failure, and there's no runtime error.
>>
>> The old client code just discards "correlation variables", and I guess it
>> would result in wrong results or something like that.
>> Silent wrong results is a really sad outcome from the database.
>>
>> CALCITE-4913 / PR#2623 adds Project#variablesSet property as well, and I
>> guess it would in the same hidden semantic loss.
>>
>> Vladimir
>>
>
>
> --
> Regards,
> Konstantin Orlov
>
--
Regards,
Konstantin Orlov
Re: RelNode semantic evolution vs adapter implementations
Posted by Konstantin Orlov <ko...@gmail.com>.
Vladimir, could you please clarify in what way the PR#2623 changes
the semantics?
The correlated project is already possible in the master. The MongoProject
already discards variablesSet (simply because it's currently not stored for
project node) and either fails or returns invalid results. This behavior
(alas,
incorrect) will be preserved after this patch.
пн, 13 дек. 2021 г. в 17:55, Vladimir Sitnikov <sitnikov.vladimir@gmail.com
>:
> Hi,
>
> It turns out Calcite's nodes can change semantics without notification for
> the end-users.
>
> Here are the release notes for Calcite 1.21, and it says **nothing** like
> "Ensure you handle Filter#variablesSet in case you implement Filter or in
> case you transform LogicalFilter in your rules"
> https://calcite.apache.org/news/2019/09/11/release-1.21.0/
>
> On top of that, the in-core adapters fail to handle that properly. For
> example, MongoFilter discards Filter#variablesSet.
>
> Can we please stop changing the semantics of RelNodes or can we have a
> better way to detect the changes in the client code?
>
> What if we add a "version" property to the corresponding RelNodes, and we
> increment it every time the semantic changes?
> Then client APIs could be coded like "ok, I'm prepared to handle Project
> v4, and Filter v5" (e.g. make "version" required when registering a rule),
> and there will be a runtime error in case Calcite generates Filter v6 in
> runtime.
>
> ---
>
> Sample case:
> CALCITE-3183 Trimming method for Filter rel uses wrong traitSet
> CALCITE-4913 Correlated variables in a select list are not deduplicated
>
> The case there is that "correlation variables" are added to the Logical*
> nodes (e.g. LogicalFilter, LogicalProject).
> Unfortunately, that change is hard to notice: there's no compilation
> failure, and there's no runtime error.
>
> The old client code just discards "correlation variables", and I guess it
> would result in wrong results or something like that.
> Silent wrong results is a really sad outcome from the database.
>
> CALCITE-4913 / PR#2623 adds Project#variablesSet property as well, and I
> guess it would in the same hidden semantic loss.
>
> Vladimir
>
--
Regards,
Konstantin Orlov
Re: RelNode semantic evolution vs adapter implementations
Posted by Vladimir Sitnikov <si...@gmail.com>.
>practice I fear that
>it will introduce a lot of red tape and false negatives
I am afraid "wrong results and corrupted data in database" is way worse
than "recompiling in case of Calcite update".
>I don't recall
>any feedback (positive or otherwise) about how we describe breaking
>changes
CALCITE-3183 was breaking in 1.21, and it was NOT listed in release notes
at all.
Everybody makes mistakes, so I would avoid using "release notes" as the
only way to convey the breaking changes.
In the ideal world, there should be a compilation failure (or runtime
failure) in case of semantic change.
>Maybe we can improve how we report breaking changes
I'm afraid people would miss the notifications.
They can upgrade across several calcite versions, so they might miss
"one release inside of 1.21...1.28 has a significant breaking change"
If we increased the major version number, then they would anticipate the
change.
However, the thing is that "the change in Project" does not necessarily
break all consumers,
and it breaks only those consumers that inspect Project contents and/or
write new methods, etc.
----
>CALCITE-3183 Trimming method for Filter
In that case, the new field was added to LogicalFilter, so custom
implementations of Filter nodes (e.g. MongoFilter)
had no chance to notice the change.
>CALCITE-4913 Correlated variables in a select list
In the current PR 2623, the extra field is added to Project.java, however,
old constructor is kept intact, so even classes that extend Project don't
notice the semantics difference.
My takeaway is that:
1) It looks like we should refrain from adding fields to "Logical*" rels
without adding the corresponding fields to base classes.
2) It looks like we want to change the constructor signature, so the user's
code breaks if they extended Project.
As an alternative option is to keep old constructors and mark them
as @Deprecated, however, that deprecation would mean
"you get wrong results unless you support the new constructor argument
somehow".
3) Unfortunately, the planner rules do not care which signature the
constructors have, so the planner rules
won't notice the semantic difference.
So I would suggest we add per-relnode "version" (or "feature set"), so the
planner rule could declare "I know what Filter up to v5 means".
We do not change node semantics often, however, when we do we want to avoid
accidental "wrong results".
Vladimir
Re: RelNode semantic evolution vs adapter implementations
Posted by Julian Hyde <jh...@apache.org>.
Vladimir's points are correct in theory, but in practice I fear that
it will introduce a lot of red tape and false negatives, and not yield
benefits in proportion to that effort.
Maybe we can improve how we report breaking changes. I don't recall
any feedback (positive or otherwise) about how we describe breaking
changes in our release notes. So it's difficult to know how we can do
better.
Julian
On Mon, Dec 13, 2021 at 9:57 AM Alessandro Solimando
<al...@gmail.com> wrote:
>
> Hi,
> what you mention really sounds like breaking changes, so they should be
> treated as such and be explicitly mentioned in the release notes.
>
> Whenever possible, these behaviours should also be covered by unit tests,
> in order to limit the chance of changes sneaking in silently.
>
> Concerning the versioning of RelNode semantics, do you expect "consuming"
> code to throw on higher semantics versions than expected?
> I see the general idea behind your proposal but I fail to imagine where
> this kind of version check should happen, in order not to pollute too much
> the "consumer" code.
>
> Best regards,
> Alessandro
>
> On Mon, 13 Dec 2021 at 15:55, Vladimir Sitnikov <si...@gmail.com>
> wrote:
>
> > Hi,
> >
> > It turns out Calcite's nodes can change semantics without notification for
> > the end-users.
> >
> > Here are the release notes for Calcite 1.21, and it says **nothing** like
> > "Ensure you handle Filter#variablesSet in case you implement Filter or in
> > case you transform LogicalFilter in your rules"
> > https://calcite.apache.org/news/2019/09/11/release-1.21.0/
> >
> > On top of that, the in-core adapters fail to handle that properly. For
> > example, MongoFilter discards Filter#variablesSet.
> >
> > Can we please stop changing the semantics of RelNodes or can we have a
> > better way to detect the changes in the client code?
> >
> > What if we add a "version" property to the corresponding RelNodes, and we
> > increment it every time the semantic changes?
> > Then client APIs could be coded like "ok, I'm prepared to handle Project
> > v4, and Filter v5" (e.g. make "version" required when registering a rule),
> > and there will be a runtime error in case Calcite generates Filter v6 in
> > runtime.
> >
> > ---
> >
> > Sample case:
> > CALCITE-3183 Trimming method for Filter rel uses wrong traitSet
> > CALCITE-4913 Correlated variables in a select list are not deduplicated
> >
> > The case there is that "correlation variables" are added to the Logical*
> > nodes (e.g. LogicalFilter, LogicalProject).
> > Unfortunately, that change is hard to notice: there's no compilation
> > failure, and there's no runtime error.
> >
> > The old client code just discards "correlation variables", and I guess it
> > would result in wrong results or something like that.
> > Silent wrong results is a really sad outcome from the database.
> >
> > CALCITE-4913 / PR#2623 adds Project#variablesSet property as well, and I
> > guess it would in the same hidden semantic loss.
> >
> > Vladimir
> >
Re: RelNode semantic evolution vs adapter implementations
Posted by Alessandro Solimando <al...@gmail.com>.
Hi,
what you mention really sounds like breaking changes, so they should be
treated as such and be explicitly mentioned in the release notes.
Whenever possible, these behaviours should also be covered by unit tests,
in order to limit the chance of changes sneaking in silently.
Concerning the versioning of RelNode semantics, do you expect "consuming"
code to throw on higher semantics versions than expected?
I see the general idea behind your proposal but I fail to imagine where
this kind of version check should happen, in order not to pollute too much
the "consumer" code.
Best regards,
Alessandro
On Mon, 13 Dec 2021 at 15:55, Vladimir Sitnikov <si...@gmail.com>
wrote:
> Hi,
>
> It turns out Calcite's nodes can change semantics without notification for
> the end-users.
>
> Here are the release notes for Calcite 1.21, and it says **nothing** like
> "Ensure you handle Filter#variablesSet in case you implement Filter or in
> case you transform LogicalFilter in your rules"
> https://calcite.apache.org/news/2019/09/11/release-1.21.0/
>
> On top of that, the in-core adapters fail to handle that properly. For
> example, MongoFilter discards Filter#variablesSet.
>
> Can we please stop changing the semantics of RelNodes or can we have a
> better way to detect the changes in the client code?
>
> What if we add a "version" property to the corresponding RelNodes, and we
> increment it every time the semantic changes?
> Then client APIs could be coded like "ok, I'm prepared to handle Project
> v4, and Filter v5" (e.g. make "version" required when registering a rule),
> and there will be a runtime error in case Calcite generates Filter v6 in
> runtime.
>
> ---
>
> Sample case:
> CALCITE-3183 Trimming method for Filter rel uses wrong traitSet
> CALCITE-4913 Correlated variables in a select list are not deduplicated
>
> The case there is that "correlation variables" are added to the Logical*
> nodes (e.g. LogicalFilter, LogicalProject).
> Unfortunately, that change is hard to notice: there's no compilation
> failure, and there's no runtime error.
>
> The old client code just discards "correlation variables", and I guess it
> would result in wrong results or something like that.
> Silent wrong results is a really sad outcome from the database.
>
> CALCITE-4913 / PR#2623 adds Project#variablesSet property as well, and I
> guess it would in the same hidden semantic loss.
>
> Vladimir
>