yyanyy commented on code in PR #57582:
URL: https://github.com/apache/spark/pull/57582#discussion_r3692897589
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/RelationResolution.scala:
##########
@@ -280,7 +281,8 @@ class RelationResolution(
catalog,
ident,
finalTimeTravelSpec,
- Option(writePrivileges))
+ Option(writePrivileges),
+ finalOptions)
Review Comment:
Thanks for the catch! I originally looked at this a little, but decided it
might not be an issue because the table returned should have the options
correctly reflected, and that would hit the cache correctly. But you're right
that I didn't realize the same-table check inside the cache lookup is based
only on the ID, and there's no guarantee the ID reflects the options; even if
the options substantially change the table content, the ID might still be the
same. So agreed, it's much safer to include the options in the lookup.
This may give up reuse in some edge cases where the cache could actually
have been reused safely, e.g. purely read-tuning options that don't change
which table is read or how it's structured; but that's still better than
risking the wrong table from the cache.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
