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

Add more const evaluation for GenericBinaryArray and GenericListArray: add PREFIX and data type constructor #2327

Merged
merged 1 commit into from Aug 5, 2022

Conversation

HaoYang670
Copy link
Contributor

Signed-off-by: remzi 13716567376yh@gmail.com

Which issue does this PR close?

Closes #2311 .

Rationale for this change

More constant evaluation is better.
Less repeated code is better.

What changes are included in this PR?

  1. Add data type constructor for list array
  2. Add const field PREFIX for OffsetSizeTrait

Are there any user-facing changes?

No.

Signed-off-by: remzi <13716567376yh@gmail.com>
@github-actions github-actions bot added arrow Changes to the arrow crate parquet Changes to the parquet crate labels Aug 5, 2022
@alamb alamb changed the title Add more const evaluation for list array Add more const evaluation for list array: PREFIX and data type constructor Aug 5, 2022
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.

This looks like a very nice cleanup to me -- thank you @HaoYang670

/// The data type constructor of list array.
/// The input is the schema of the child array and
/// the output is the [`DataType`], List or LargeList.
pub const DATA_TYPE_CONSTRUCTOR: fn(Box<Field>) -> DataType = if OffsetSize::IS_LARGE
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub const DATA_TYPE_CONSTRUCTOR: fn(Box<Field>) -> DataType = if OffsetSize::IS_LARGE
pub const MAKE_DATA_TYPE: fn(Box<Field>) -> DataType = if OffsetSize::IS_LARGE

DATA_TYPE_CONSTRUCTOR was somewhat hard on my eyes when reading this PR. That being said while I might prefer something like MAKE_DATA_TYPE it is only an opinion for your consideration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review @alamb. Personally, I prefer the name CONSTRUCTOR, as ListArray and LargeListArray constructor tags of DataType: https://en.wikipedia.org/wiki/Algebraic_data_type#:~:text=A%20general%20algebraic%20data%20type,factors%20of%20the%20product%20type.

use std::any::TypeId;

let mut offset = vec![0];
let mut offset = vec![S::zero()];
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@alamb alamb added the api-change Changes to the arrow API label Aug 5, 2022
@alamb alamb changed the title Add more const evaluation for list array: PREFIX and data type constructor Add more const evaluation for GenericBinaryArray and GenericListArray: add PREFIX and data type constructor Aug 5, 2022
@alamb
Copy link
Contributor

alamb commented Aug 5, 2022

I am going to take this one into Arrow 20 as it is also an API change -- I am hoping one of these releases we'll be able to do a minor one ;)

@alamb alamb merged commit b1e2bd9 into apache:master Aug 5, 2022
@ursabot
Copy link

ursabot commented Aug 5, 2022

Benchmark runs are scheduled for baseline = 3ed0e28 and contender = b1e2bd9. b1e2bd9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@HaoYang670 HaoYang670 deleted the add_list_datatype_constructor branch August 5, 2022 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Changes to the arrow API arrow Changes to the arrow crate parquet Changes to the parquet crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the constant data type constructors for ListArray
3 participants