Skip to content

[SPARK-51771][SQL][FOLLOWUP] Rename currentVersion to version in DSv2 Table #53118

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[SPARK-51771][SQL][FOLLOWUP] Rename currentVersion to version in DSv2 Table #53118

wants to merge 2 commits into from
+33 −31

Conversation

@aokolnychyi
Copy link
Contributor

@aokolnychyi aokolnychyi commented Nov 18, 2025

What changes were proposed in this pull request?

This PR renames currentVersion to version in DSv2 Table. This method is supposed to be a simple getter and must not trigger a refresh of the underlying table state.

Why are the changes needed?

These changes are needed to avoid ambiguity in the about to be released API in 4.1.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests.

Was this patch authored or co-authored using generative AI tooling?

No.

@github-actions github-actions bot added the SQL label Nov 18, 2025
dongjoon-hyun
dongjoon-hyun reviewed Nov 18, 2025
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @gengliangwang , @viirya , @cloud-fan , @szehon-ho from the original PR.

viirya
viirya reviewed Nov 18, 2025
View reviewed changes
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/Table.java Outdated

/**
* Returns the current table version if implementation supports versioning.
* Returns this table version without refreshing state if implementation supports versioning.
Copy link
Member

@viirya viirya Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, can you explain more why to specify specially on "without refreshing state"? Does currentVersion implicitly refresh state possibly? I suppose currentVersion just returns the current version, and don't expect it will refresh state.

Copy link
Member

@gengliangwang gengliangwang Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, without refreshing state is confusing

Copy link
Member

@gengliangwang gengliangwang Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about

/**
 * Returns the current table version if the implementation supports versioning.
 * The returned value is not automatically refreshed.
 * If the table is not versioned, this method may return null.
 */

Copy link
Contributor

@cloud-fan cloud-fan Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor

@cloud-fan cloud-fan Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The returned value -> This table instance

Copy link
Contributor Author

@aokolnychyi aokolnychyi Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the whole purpose of the rename is to make sure connectors DO NOT automatically refresh the underlying table state. In other words, it must act like a guidance for connectors.

Copy link
Contributor Author

@aokolnychyi aokolnychyi Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this, folks?

/**
 * Returns the version of this table if versioning is supported, null otherwise.
 * <p>
 * This method must not trigger a refresh of the table metadata. It should return
 * the version that corresponds to the current state of this table instance.
 */

Copy link
Contributor Author

@aokolnychyi aokolnychyi Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to avoid the word current as this may not be current and can be behind the version in the metastore.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Nov 20, 2025

Gentle ping, @aokolnychyi .

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Nov 20, 2025

Please address the review comments and get approvals if you really want to make this change at Apache Spark 4.1.0, @aokolnychyi .

@aokolnychyi
Copy link
Contributor Author

aokolnychyi commented Nov 20, 2025

@dongjoon-hyun, updated. Sorry about the delay, I was updating other PRs too.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Nov 20, 2025

Thank you, @aokolnychyi .

dongjoon-hyun
dongjoon-hyun approved these changes Nov 20, 2025
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM.

dongjoon-hyun pushed a commit that referenced this pull request Nov 20, 2025
@aokolnychyi @dongjoon-hyun
[SPARK-51771][SQL][FOLLOWUP] Rename currentVersion to version in DSv2…
7575478 
… Table

### What changes were proposed in this pull request?

This PR renames `currentVersion` to `version` in DSv2 `Table`. This method is supposed to be a simple getter and must not trigger a refresh of the underlying table state.

### Why are the changes needed?

These changes are needed to avoid ambiguity in the about to be released API in 4.1.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Existing tests.

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes #53118 from aokolnychyi/spark-51771-followup.

Authored-by: Anton Okolnychyi <aokolnychyi@apache.org>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit 6578b9b)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
@aokolnychyi
Copy link
Contributor Author

aokolnychyi commented Nov 20, 2025

I don't think the test failure is relevant:

[info]   org.apache.spark.memory.SparkOutOfMemoryError
[info]   {
[info]     "errorClass" : "UNABLE_TO_ACQUIRE_MEMORY",
[info]     "sqlState" : "53200",
[info]     "messageParameters" : {
[info]       "receivedBytes" : "0",
[info]       "requestedBytes" : "262144"
[info]     }
[info]   } (SQLQueryTestSuite.scala:681)

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Nov 20, 2025

Merged to master/4.1 for Apache Spark 4.1.0.

@aokolnychyi
Copy link
Contributor Author

aokolnychyi commented Nov 20, 2025

Thank you, @dongjoon-hyun @gengliangwang @cloud-fan @viirya!

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Nov 20, 2025 •
edited
Loading

Ya, @aokolnychyi . I checked the first commit and second commit result. The failed ones are different. They look like flaky ones.

zifeif2 pushed a commit to zifeif2/spark that referenced this pull request Nov 25, 2025
@aokolnychyi @zifeif2
[SPARK-51771][SQL][FOLLOWUP] Rename currentVersion to version in DSv2…
fa48c3c 
… Table

### What changes were proposed in this pull request?

This PR renames `currentVersion` to `version` in DSv2 `Table`. This method is supposed to be a simple getter and must not trigger a refresh of the underlying table state.

### Why are the changes needed?

These changes are needed to avoid ambiguity in the about to be released API in 4.1.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Existing tests.

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes apache#53118 from aokolnychyi/spark-51771-followup.

Authored-by: Anton Okolnychyi <aokolnychyi@apache.org>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
huangxiaopingRD pushed a commit to huangxiaopingRD/spark that referenced this pull request Nov 25, 2025
@aokolnychyi @huangxiaopingRD
[SPARK-51771][SQL][FOLLOWUP] Rename currentVersion to version in DSv2…
7bdd9af 
… Table

### What changes were proposed in this pull request?

This PR renames `currentVersion` to `version` in DSv2 `Table`. This method is supposed to be a simple getter and must not trigger a refresh of the underlying table state.

### Why are the changes needed?

These changes are needed to avoid ambiguity in the about to be released API in 4.1.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Existing tests.

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes apache#53118 from aokolnychyi/spark-51771-followup.

Authored-by: Anton Okolnychyi <aokolnychyi@apache.org>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fan cloud-fan cloud-fan left review comments

@viirya viirya viirya approved these changes

@gengliangwang gengliangwang gengliangwang approved these changes

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@aokolnychyi @dongjoon-hyun @viirya @gengliangwang @cloud-fan