dssysolyatin commented on code in PR #4620:
URL: https://github.com/apache/calcite/pull/4620#discussion_r2517293520
##########
core/src/test/resources/org/apache/calcite/test/SqlHintsConverterTest.xml:
##########
@@ -373,7 +371,6 @@ TableScan:[[PROPERTIES inheritPath:[] options:{K1=v1,
K2=v2}], [PROPERTIES inher
<Resource name="hints">
<![CDATA[
Project:[[FAST_SNAPSHOT inheritPath:[] options:[PRODUCTS_TEMPORAL]]]
-Snapshot:[[FAST_SNAPSHOT inheritPath:[0, 1] options:[PRODUCTS_TEMPORAL]]]
Review Comment:
@xuzifu666 You didn’t answer my question. The `RelShuttle` interface already
has a method `RelShuttle.visit(RelNode other)`. So why need new method
`RelShuttle.visit(LogicalSnapshot)` to this interface? You can just keep your
changes in LogicalSnapshot, but not add new method inside `RelShuttle` interface
```
@Override public RelNode accept(RelShuttle shuttle) {
return shuttle.visit(this); <-- this calls visit(RelNode other)
}
```
When you add a new method, all LogicalSnapshot related code will start
calling `RelShuttle.visit(LogicalSnapshot)` instead of
`RelShuttle.visit(RelNode other)`. But users might already handle snapshots in
their `RelShuttle.visit(RelNode other)` method (as it happens with
`HintCollector`/`RelHomogeneousShuttle`). It can silently break their code.
Even if they have tests, they might think everything is fine, just like you did
with the current tests while some issues still go unnoticed. As a result, it
could take them a long time to discover and fix the problem. How much time did
you spend fixing all the issues caused by this change ?
I still think we shouldn’t add a new method. But if you do, please list it
as a breaking change in the release notes (site/_docs/history.md), since it
changes the behavior. See the example in Stamatis’s commit
https://github.com/apache/calcite/pull/4625/commits/1ab5c7f8cc54a6df87dafe5371530443fa810e19#diff-c0eccc3130d0bc82b14113843abec157190ebb2551380fa754dbcdd56f61ea51
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
