dssysolyatin commented on code in PR #4620:
URL: https://github.com/apache/calcite/pull/4620#discussion_r2515800009


##########
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:
   This test checks hint propagation. Your changes shouldn't affect anything 
here. Double-check every Shuttle that extends RelShuttleImpl. There's probably 
one in the test.
   
   Also please add a note to the release notes for the next release. If someone 
extends `RelShuttleImpl` and depends on `RelShuttleImpl.visit(RelNode other)` 
for `Snapshot`, they will need to update their code to override the new method. 
This is one of example of poor code design. 
   
   UPD: I checked a code and there is `HintCollector` inside 
`SqlHintsConverterTest` which as I said rely on `RelShuttleImpl.visit(RelNode 
other)` for snapshot. Add your method and it should fix test



-- 
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]

Reply via email to