Skip to content

Core: Refactor ZOrderByteUtils #10624

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
RussellSpitzer merged 1 commit into from
Sep 3, 2024
Merged

Core: Refactor ZOrderByteUtils #10624

RussellSpitzer merged 1 commit into from
Sep 3, 2024

Conversation

@ajantha-bhat
Copy link
Member

@ajantha-bhat ajantha-bhat commented Jul 3, 2024 •
edited
Loading

Follow up from #10583 (comment)
Addressing my own comment.

  • Fix the java docs
  • Update intToOrderedBytes to use longToOrderedBytes as it is the same code.
  • Use longToOrderedBytes for shortToOrderedBytes and tinyintToOrderedBytes to avoid double casting.
  • Extract common function to new public methods.

@github-actions github-actions bot added the core label Jul 3, 2024
@ajantha-bhat
Copy link
Member Author

ajantha-bhat commented Jul 3, 2024

cc: @snazy, @nastra

@ajantha-bhat ajantha-bhat requested a review from nastra July 4, 2024 10:22
@ajantha-bhat
Copy link
Member Author

ajantha-bhat commented Jul 17, 2024

ping: @nastra

@nastra nastra requested review from RussellSpitzer and removed request for nastra July 17, 2024 13:19
@RussellSpitzer
Copy link
Member

RussellSpitzer commented Jul 18, 2024

I think this is generally fine, but I do think the internal methods being called should probably be renamed rather that continuing to use the "long" and "double" method names.

for example having a
"wholeNumberOrderedBytes"

and

"floatingPointOrderedBytes"

@ajantha-bhat
Copy link
Member Author

ajantha-bhat commented Jul 31, 2024

@RussellSpitzer: Thanks for the review. I have extracted the inner method as you suggested.

@ajantha-bhat
Copy link
Member Author

ajantha-bhat commented Aug 1, 2024

Failed due to flaky test: #10599

@ajantha-bhat ajantha-bhat reopened this Aug 1, 2024
RussellSpitzer
RussellSpitzer requested changes Aug 1, 2024
View reviewed changes
Copy link
Member

@RussellSpitzer RussellSpitzer left a comment

Choose a reason for hiding this comment

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

Sorry, I missed that the java docs all need to be fixed now. They now reference things which aren't happening anymore. The implementation details should go in the doc for wholeNumberedOrderedBytes and FloatingPOintOrderedBytes

@ajantha-bhat
Copy link
Member Author

ajantha-bhat commented Sep 2, 2024

@RussellSpitzer: I have updated it today. Sorry for delay on this. I lost track of this PR notification.

@RussellSpitzer
Copy link
Member

RussellSpitzer commented Sep 3, 2024

Thanks @ajantha-bhat , Merged

@RussellSpitzer RussellSpitzer merged commit 5319767 into apache:main Sep 3, 2024
jenbaldwin pushed a commit to Teradata/iceberg that referenced this pull request Sep 17, 2024
Merge branch 'main' into feature/OTF-1500_column_comparison
7e275b0 
* main: (208 commits)
  Docs: Fix Flink 1.20 support versions (apache#11065)
  Flink: Fix compile warning (apache#11072)
  Docs: Initial committer guidelines and requirements for merging (apache#10780)
  Core: Refactor ZOrderByteUtils (apache#10624)
  API: implement types timestamp_ns and timestamptz_ns (apache#9008)
  Build: Bump com.google.errorprone:error_prone_annotations (apache#11055)
  Build: Bump mkdocs-material from 9.5.33 to 9.5.34 (apache#11062)
  Flink: Backport PR apache#10526 to v1.18 and v1.20 (apache#11018)
  Kafka Connect: Disable publish tasks in runtime project (apache#11032)
  Flink: add unit tests for range distribution on bucket partition column (apache#11033)
  Spark 3.5: Use FileGenerationUtil in PlanningBenchmark (apache#11027)
  Core: Add benchmark for appending files (apache#11029)
  Build: Ignore benchmark output folders across all modules (apache#11030)
  Spec: Add RemovePartitionSpecsUpdate REST update type (apache#10846)
  Docs: bump latest version to 1.6.1 (apache#11036)
  OpenAPI, Build: Apply spotless to testFixtures source code (apache#11024)
  Core: Generate realistic bounds in benchmarks (apache#11022)
  Add REST Compatibility Kit (apache#10908)
  Flink: backport PR apache#10832 of inferring parallelism in FLIP-27 source (apache#11009)
  Docs: Add Druid docs url to sidebar (apache#10997)
  ...
zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024
@ajantha-bhat @zachdisc
Core: Refactor ZOrderByteUtils (apache#10624)
a01ea69
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RussellSpitzer RussellSpitzer RussellSpitzer approved these changes

Assignees

No one assigned

Labels

core

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ajantha-bhat @RussellSpitzer