Core: Fix API breakages around scanMetrics()#6664
Conversation
There was a problem hiding this comment.
Hi @nastra , thanks for this fix. Sorry initially I intended just to move the method to the super class.
I got confused that I'm back at Java from Scala, I somehow it slipped my mind and I thought making it from protected => default is a backward compatibile promotion :(. It seems I dont need to change the method access level then, and I'm ok with leaving it as protected. Do we still need the deprecation as its still available on BaseTableScan's subclasses via inheritance (and was not really a public API to begin with?)
nastra
force-pushed
the
fix-scan-metrics-revapi-breaks
branch
from
0965fb3 to
e900425
Compare
|
@szehon-ho thanks for the review. I double-checking and we actually don't need to deprecate anything. Making the method in the super class |
szehon-ho
left a comment
szehon-ho
left a comment
There was a problem hiding this comment.
Looks good to me, if test pass. Thanks!
ajantha-bhat
left a comment
ajantha-bhat
left a comment
There was a problem hiding this comment.
LGTM.
Please update the PR description. It still talks about deprecation.
|
Merged, thanks @nastra and @ajantha-bhat for review |
3 participants
#6365 moved
scanMetrics()fromBaseTableScanhigher in the hierarchy toSnapshotScanand required RevAPI changes due to making method package-private instead ofprotected(meaning that classes in different packages wouldn't have access to that method anymore). This PR makes sure that we don't need the RevAPI changes and changes visibility back toprotected./cc @szehon-ho @rdblue