Skip to content
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

Session properties in doc don't match with the implementation #19826

Closed
oneonestar opened this issue Nov 20, 2023 · 6 comments · Fixed by #20560
Closed

Session properties in doc don't match with the implementation #19826

oneonestar opened this issue Nov 20, 2023 · 6 comments · Fixed by #20560
Labels

Comments

@oneonestar
Copy link
Member

oneonestar commented Nov 20, 2023

After ran through all the session properties in doc, the following properties don't match with the implementation:

Doc Impl
push_aggregation_through_join push_aggregation_through_outer_join
min_hash_partition_count_for_writre min_hash_partition_count_for_write
query_remote_task_enable_adaptive_request_size remote_task_adaptive_update_request_size_enabled
query_remote_task_guaranteed_splits_per_task remote_task_guaranteed_splits_per_request
query_remote_task_max_request_size remote_task_max_request_size
query_remote_task_request_size_headroom remote_task_request_size_headroom

Ref:


Extract all session properties in doc

$ grep --include=\*.md -rn docs/src/main/sphinx -e '\*\*Session property:'
docs/src/main/sphinx/admin/properties-spilling.md:9:- **Session property:** `spill_enabled`
docs/src/main/sphinx/admin/properties-write-partitioning.md:9:- **Session property:** `use_preferred_write_partitioning`
....
@ebyhr
Copy link
Member

ebyhr commented Jan 24, 2024

cc: @mosabua

@mosabua
Copy link
Member

mosabua commented Jan 24, 2024

Great job finding this. Do you plan on sending a PR to fix all of these @oneonestar ?

@oneonestar
Copy link
Member Author

oneonestar commented Jan 25, 2024

Fix the first two items in #20461.
Changes on remote_task_* require some discussion because I think the implementation should be changed to keep naming consistent. This will be a breaking change. Should I open another issue or can we just discuss here?

@mosabua
Copy link
Member

mosabua commented Jan 25, 2024

I think we should just fix the docs .. and if we want to rename the properties that is a separate topic to discuss with various others. We can tackle that afterwards.

@oneonestar
Copy link
Member Author

#20075 (comment)

the names are constructed hierarchical

Referring to this comment, these session properties should follow the naming convention.
Thus the implementation should be fixed instead of the doc.

@mosabua
Copy link
Member

mosabua commented Jan 26, 2024

At this stage we dont have agreement on the renaming .. but we do have agreement that the docs are wrong and should be fixed. So lets fix the docs for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

3 participants