8000 [Serializer] Lambda Compatibilty Fix by maiadegraaf · Pull Request #17428 · duckdb/duckdb · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Serializer] Lambda Compatibilty Fix #17428

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
Merged
merged 4 commits into from
May 14, 2025
Merged

[Serializer] Lambda Compatibilty Fix #17428

merged 4 commits into from
May 14, 2025

Conversation

maiadegraaf
Copy link
Contributor

#16602 introduced a new member to ListLambdaBindData; however, a default value was not set for the serializer/deserializer, which made it incompatible with previous versions. This has now been fixed.

@maiadegraaf maiadegraaf requested a review from taniabogatsch May 9, 2025 14:07
taniabogatsch
taniabogatsch reviewed May 9, 2025
View reviewed changes
Copy link
Contributor
@taniabogatsch taniabogatsch left a comment

Choose a reason for hiding this comment

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

Is there a way to include the serialised plan (and reproduction) in this PR? How do we do this with other serialised plans? Do we have a folder for this similar to the data folder for files with the older storage format?

I'd like to see some type of test in this PR.

@Mytherin
Copy link
Collaborator

Is there a way to include the serialised plan (and reproduction) in this PR? How do we do this with other serialised plans? Do we have a folder for this similar to the data folder for files with the older storage format?

I'd like to see some type of test in this PR.

We would need to run an older DuckDB version that reads a database file produced by a new DuckDB version. This is possible but that doesn't exist in our test suite right now.

For this PR, maybe we can add a test that creates a view in a persistent database that has a list_reduce function with an initial value?

@duckdb-draftbot duckdb-draftbot marked this pull request as draft May 13, 2025 12:39
@maiadegraaf maiadegraaf marked this pull request as ready for review May 13, 2025 12:39
@maiadegraaf
Copy link
Contributor Author

Thanks for your feedback! I've added a test that attaches an older storage version.

@maiadegraaf maiadegraaf force-pushed the lamdba-serialization-bug branch from 2fbea7e to a765001 Compare May 13, 2025 13:08
@duckdb-draftbot duckdb-draftbot marked this pull request as draft May 13, 2025 13:09
@maiadegraaf maiadegraaf marked this pull request as ready for review May 13, 2025 13:13
Mytherin
Mytherin reviewed May 13, 2025
View reviewed changes
@duckdb-draftbot duckdb-draftbot marked this pull request as draft May 14, 2025 07:26
@maiadegraaf
Copy link
Contributor Author

I removed the conditional, so now has_initial is always serialized. I tested it locally, and it works as expected.

@maiadegraaf maiadegraaf marked this pull request as ready for review May 14, 2025 07:28
@Mytherin
Copy link
Collaborator

Thanks!

@Mytherin Mytherin merged commit 4949adc into duckdb:main May 14, 2025
52 checks passed
@maiadegraaf maiadegraaf deleted the lamdba-serialization-bug branch May 14, 2025 10:54
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 18, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@4949adc
222187a 
[Serializer] Lambda Compatibilty Fix (duckdb/duckdb#17428)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 19, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@4949adc
e6f7c61 
[Serializer] Lambda Compatibilty Fix (duckdb/duckdb#17428)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 19, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@4949adc
2573ac7 
[Serializer] Lambda Compatibilty Fix (duckdb/duckdb#17428)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mytherin Mytherin Mytherin left review comments

@taniabogatsch taniabogatsch taniabogatsch left review comments

Assignees
No one assigned
Labels
Ready To Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maiadegraaf @Mytherin @taniabogatsch
0