Skip to content

refactor lexico sort for future code reuse#423

Merged
alamb merged 1 commit into
apache:masterfrom
jimexist:refactor-lexico-sort
Jun 8, 2021
Merged

refactor lexico sort for future code reuse#423
alamb merged 1 commit into
apache:masterfrom
jimexist:refactor-lexico-sort

Conversation

@jimexist

@jimexist jimexist commented Jun 8, 2021

Copy link
Copy Markdown
Member

Which issue does this PR close?

Re #428

Rationale for this change

the logic to sort a list of columns lexicographically given sort options was previously embedded within lexsort but this pull request changes to pull that part out to a generic struct for future code reuse.

This change is used in #424

What changes are included in this PR?

Are there any user-facing changes?

@jimexist jimexist force-pushed the refactor-lexico-sort branch from c9e23b1 to efef737 Compare June 8, 2021 04:02

@jorgecarleitao jorgecarleitao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @jimexist ! Ready to go in my opinion

@jimexist

jimexist commented Jun 8, 2021

Copy link
Copy Markdown
Member Author

Thanks @jimexist ! Ready to go in my opinion

not sure what i can do about the docker build failures

@jimexist jimexist force-pushed the refactor-lexico-sort branch from efef737 to a0ba084 Compare June 8, 2021 08:11
@codecov-commenter

codecov-commenter commented Jun 8, 2021

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.64%. Comparing base (18c804a) to head (a0ba084).

Files with missing lines Patch % Lines
arrow/src/compute/kernels/sort.rs 92.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #423      +/-   ##
==========================================
- Coverage   82.64%   82.64%   -0.01%     
==========================================
  Files         162      162              
  Lines       44542    44549       +7     
==========================================
+ Hits        36813    36817       +4     
- Misses       7729     7732       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jimexist

jimexist commented Jun 8, 2021

Copy link
Copy Markdown
Member Author

Thanks @jimexist ! Ready to go in my opinion

not sure what i can do about the docker build failures

seems like rebasing just solved this issue

@alamb

alamb commented Jun 8, 2021

Copy link
Copy Markdown
Contributor

seems like rebasing just solved this issue

I think that causes the tests to get re-run and thus if the error was due to some network glitch or something it will be solved by the re-execution

@alamb alamb merged commit a37dd4f into apache:master Jun 8, 2021
@jimexist jimexist deleted the refactor-lexico-sort branch June 9, 2021 00:55
alamb pushed a commit that referenced this pull request Jun 9, 2021
alamb added a commit that referenced this pull request Jun 10, 2021
Co-authored-by: Jiayu Liu <Jimexist@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants