>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

Reply via email to