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

Remove use of deprecated dict_id in datafusion-proto (#14173) #14227

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

cj-zhukov
Copy link
Contributor

Which issue does this PR close?

Closes #14173.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) common Related to common crate proto Related to proto crate labels Jan 22, 2025
@github-actions github-actions bot removed the common Related to common crate label Jan 29, 2025
@github-actions github-actions bot removed the optimizer Optimizer rules label Jan 30, 2025
@github-actions github-actions bot removed the physical-expr Physical Expressions label Jan 30, 2025
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

It seems that datafusion/core/src/physical_optimizer/enforce_sorting.rs somehow is added to this PR accidentally (it was moved on main recently, so perhaps this happened during a merge accidentally?)

Thanks for working on this @cj-zhukov

@cj-zhukov
Copy link
Contributor Author

It seems that datafusion/core/src/physical_optimizer/enforce_sorting.rs somehow is added to this PR accidentally (it was moved on main recently, so perhaps this happened during a merge accidentally?)

Thanks for working on this @cj-zhukov

Andrew, I'm afraid it was done accidentally

@@ -108,8 +108,7 @@ message Field {
// for complex data types like structs, unions
repeated Field children = 4;
map<string, string> metadata = 5;
int64 dict_id = 6;
bool dict_ordered = 7;
bool dict_ordered = 6;
Copy link
Contributor

Choose a reason for hiding this comment

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

is dict_ordered still used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, but we can open PR and handle it. I'm ready to work on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome -- thanks. Let's get this PR ready to go and then we can work on remvoing that as a follow on

@github-actions github-actions bot removed the core Core DataFusion crate label Jan 31, 2025
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @cj-zhukov -- this is looking close

I also took the liberty of merging up from main and removing the accidentally added file

datafusion/sqllogictest/test_files/copy.slt Outdated Show resolved Hide resolved
@github-actions github-actions bot added documentation Improvements or additions to documentation sql SQL Planner development-process Related to development process of DataFusion logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate common Related to common crate functions labels Feb 3, 2025
@cj-zhukov cj-zhukov force-pushed the cj-zhukov/Remove-use-of-deprecated-dict_id-in-datafusion-proto branch from 14c1e4d to 07157a7 Compare February 3, 2025 10:22
@github-actions github-actions bot removed documentation Improvements or additions to documentation sql SQL Planner development-process Related to development process of DataFusion logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate common Related to common crate functions labels Feb 3, 2025
@cj-zhukov
Copy link
Contributor Author

@alamb My apologies, I was trying to undo deletion of test in copy.slt with 07157a7 commit but accidentally added some PRs that should not be included here

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @cj-zhukov

@andygrove are we happy that dict_id is no longer needed in DataFusion?

cc @thinkharderdev and @avantgardnerio who I think may have contributed the code for field_id originally

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proto Related to proto crate sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove use of deprecated dict_id in datafusion-proto
2 participants