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 <[email protected]> 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 >
