-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-53720][SQL] Simplify extracting Table from DataSourceV2Relation #52460
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
5a7a2f9
to
7e08697
Compare
override protected def stringArgs: Iterator[Any] = stringArgsVal.iterator | ||
} | ||
|
||
object DataSourceV2Table { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Since it extracts v2 table from the relation, i would prefer it to be ExtractV2Table
, which makes it explict that it is just an extractor and not a top level class (like org.apache.spark.sql.catalog.Table
), and follows other clases like ExtractEquiJoinKeys
, ExtractJoinWithBuckets
etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went back and forth on this one. Probably, ExtractXXX
is a bit more direct in this case. Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit, looks good otherwise
7e08697
to
ba656aa
Compare
override protected def stringArgs: Iterator[Any] = stringArgsVal.iterator | ||
} | ||
|
||
object ExtractTable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ExtractV2Table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for ExtractV2Table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM.
Could you address the last comment, @aokolnychyi ? |
What changes were proposed in this pull request?
This PR adds a new extractor for
Table
fromDataSourceV2Relation
.Why are the changes needed?
As we see over time,
DataSourceV2Relation
continues to evolve and has many args. Frequently, we only need to get the table instance and are not interested in other arguments. Therefore, it makes sense to add a new extractor for this common use case. In particular, I plan to addTimeTravelSpec
and I don't want to update a ton of places for no good reason.Does this PR introduce any user-facing change?
No.
How was this patch tested?
This PR relies on existing tests.
Was this patch authored or co-authored using generative AI tooling?
No.