Skip to content

Conversation

@ulysses-you
Copy link
Contributor

What changes were proposed in this pull request?

Add TimeTravelSpec to the key of relation cache in AnalysisContext.

Why are the changes needed?

Correct the relation resolution for the same table but different TimeTravelSpec.

Does this PR introduce any user-facing change?

yes, bug fix

How was this patch tested?

add test

@github-actions github-actions bot added the SQL label Nov 17, 2022
@ulysses-you
Copy link
Contributor Author

cc @cloud-fan @huaxingao

Copy link

@singhpk234 singhpk234 left a comment

Choose a reason for hiding this comment

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

Nice catch !

@cloud-fan
Copy link
Contributor

there is another cache in SessionCatalog.tableRelationCache, shall we update it as well?

@ulysses-you
Copy link
Contributor Author

SessionCatalog is used for v1 catalog (i.e. hive), I do not think that supports time travel

@huaxingao
Copy link
Contributor

LGTM. Thanks for fixing this! @ulysses-you

@cloud-fan
Copy link
Contributor

cloud-fan commented Nov 21, 2022

thanks, merging to master/3.3!

@cloud-fan
Copy link
Contributor

cloud-fan commented Nov 21, 2022

Oh wait a minute. Due to the spark catalog extension (set via spark.sql.catalog.spark_catalog), we can have tables supporting time travel in the v1 catalog as well. I think SessionCatalog.tableRelationCache also need a fix

Update: this cache is only used by FindDataSourceTable which is only for v1 table. V1 table doesn't support time travel.

@cloud-fan
Copy link
Contributor

thanks, merging to master/3.3!

@cloud-fan cloud-fan closed this in 07427b8 Nov 21, 2022
@ulysses-you ulysses-you deleted the time-travel-spec branch November 21, 2022 10:22
@cloud-fan
Copy link
Contributor

it has conflicts with 3.3, @ulysses-you can you create a backport PR? thanks!

@ulysses-you
Copy link
Contributor Author

thank you all, created #38741 for branch-3.3

cloud-fan pushed a commit that referenced this pull request Nov 22, 2022
…ime travel spec

backport #38687 for branch-3.3

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

Add TimeTravelSpec to the key of relation cache in AnalysisContext.

### Why are the changes needed?

Correct the relation resolution for the same table but different TimeTravelSpec.

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

yes, bug fix

### How was this patch tested?

add test

Closes #38741 from ulysses-you/time-travel-spec-3.3.

Authored-by: ulysses-you <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
SandishKumarHN pushed a commit to SandishKumarHN/spark that referenced this pull request Dec 12, 2022
…ravel spec

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

Add TimeTravelSpec to the key of relation cache in AnalysisContext.

### Why are the changes needed?

Correct the relation resolution for the same table but different TimeTravelSpec.

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

yes, bug fix

### How was this patch tested?

add test

Closes apache#38687 from ulysses-you/time-travel-spec.

Authored-by: ulysses-you <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
beliefer pushed a commit to beliefer/spark that referenced this pull request Dec 15, 2022
…ravel spec

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

Add TimeTravelSpec to the key of relation cache in AnalysisContext.

### Why are the changes needed?

Correct the relation resolution for the same table but different TimeTravelSpec.

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

yes, bug fix

### How was this patch tested?

add test

Closes apache#38687 from ulysses-you/time-travel-spec.

Authored-by: ulysses-you <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
beliefer pushed a commit to beliefer/spark that referenced this pull request Dec 18, 2022
…ravel spec

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

Add TimeTravelSpec to the key of relation cache in AnalysisContext.

### Why are the changes needed?

Correct the relation resolution for the same table but different TimeTravelSpec.

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

yes, bug fix

### How was this patch tested?

add test

Closes apache#38687 from ulysses-you/time-travel-spec.

Authored-by: ulysses-you <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants