-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
N-ary lambdas, and indexes as lambda parameters #8851
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…standard vector size
# Conflicts: # src/core_functions/lambda_functions.cpp
# Conflicts: # src/core_functions/scalar/list/CMakeLists.txt # src/planner/binder/expression/bind_function_expression.cpp
# Conflicts: # src/core_functions/lambda_functions.cpp # src/function/scalar_function.cpp # src/include/duckdb/planner/expression/bound_lambda_expression.hpp # src/planner/expression/bound_lambdaref_expression.cpp # src/storage/serialization/serialize_parsed_expression.cpp
maiadegraaf
reviewed
Sep 18, 2023
# Conflicts: # src/storage/serialization/serialize_expression.cpp
Mytherin
reviewed
Oct 9, 2023
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.
Thanks for the PR! Looks good - one minor comment:
Note to myself: these two branches have pending PRs with bug fixes after this major PR is in. |
Thanks! |
1 task
krlmlr
added a commit
to duckdb/duckdb-r
that referenced
this pull request
Dec 11, 2023
Merge pull request duckdb/duckdb#9164 from Mause/feature/jdbc-uuid-param Merge pull request duckdb/duckdb#9185 from pdet/adbc_07 Merge pull request duckdb/duckdb#9126 from Maxxen/parquet-kv-metadata Merge pull request duckdb/duckdb#9123 from lnkuiper/parquet_schema Merge pull request duckdb/duckdb#9086 from lnkuiper/json_inconsistent_structure Merge pull request duckdb/duckdb#8977 from Tishj/python_readcsv_multi_v2 Merge pull request duckdb/duckdb#9279 from hawkfish/nsdate-cast Merge pull request duckdb/duckdb#8851 from taniabogatsch/binary_lambdas Merge pull request duckdb/duckdb#8983 from Maxxen/types/fixedsizelist Merge pull request duckdb/duckdb#9318 from Maxxen/fix-unused Merge pull request duckdb/duckdb#9220 from hawkfish/exclude Merge pull request duckdb/duckdb#9230 from Maxxen/json-plan-serialization Merge pull request duckdb/duckdb#9011 from Tmonster/add_create_statement_support_to_fuzzer Merge pull request duckdb/duckdb#9400 from Maxxen/array-fixes Merge pull request duckdb/duckdb#8741 from Tishj/python_import_cache_upgrade Merge fixes Merge pull request duckdb/duckdb#9395 from taniabogatsch/lambda-performance Merge pull request duckdb/duckdb#9427 from Tishj/python_table_support_replacement_scan Merge pull request duckdb/duckdb#9516 from carlopi/fixformat Merge pull request duckdb/duckdb#9485 from Maxxen/fix-parquet-serialization Merge pull request duckdb/duckdb#9388 from chrisiou/issue217 Merge pull request duckdb/duckdb#9565 from Maxxen/fix-array-vector-sizes Merge pull request duckdb/duckdb#9583 from carlopi/feature Merge pull request duckdb/duckdb#8907 from cryoEncryp/new-list-functions Merge pull request duckdb/duckdb#8642 from Virgiel/capi-streaming-arrow Merge pull request duckdb/duckdb#8658 from Tishj/pytype_optional Merge pull request duckdb/duckdb#9040 from Light-City/feature/set_mg
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this PR, our lambda functions must always be unary, i.e., the list of lambda parameters is just a single parameter:
x -> expr
. This PR reworks the lambda binding functionality to allow for n-ary lambda parameters. This change lays the groundwork to supportlist_reduce
in a future PR.None of our lambda functions (
list_transform
,list_filter
) require more than a single parameter. However, allowing n-ary lambdas enables supporting an index parameter to our existing lambda functions. This index parameter must always be the second parameter, containing the 1-based index of the current element in the list.Scalar function callback
To correctly assign the types of the different lambda parameters, we need to support callbacks to the specific scalar functions. Only these functions know precisely how their parameters will look (e.g., the first parameter is the list child, and the second is
UBIGINT
for indexes).Additional Notes
list_transform
andlist_filter
. This is only an initial refacto,r and we should pick this up in a future PR (more code separation, no more flatten, more const).LAMBDA_REF
expression to better distinguish between column and lambda references during binding.