builder options and layout builder updates #1560

ianna · 2022-07-22T15:48:26Z

ArrayBuilder, LayoutBuilder, and GrowableBuffer are using BuildOptions now. There two options defined at the moment - an initial which is int64_t and a resize factor which is double. The Options is a tuple and can accept other parameters. A test is added.

This PR also drops requirements on C++ version for LayoutBuilder from C++17 to C++14. Both GrowableBuffer and BuildOptions can be used with C++11.

BitMasked mask is fixed.

codecov · 2022-07-22T16:04:16Z

Codecov Report

Merging #1560 (65a73ee) into main (78a6535) will increase coverage by 0.24%.
The diff coverage is 73.86%.

Impacted Files	Coverage Δ
src/awkward/_v2/contents/indexedoptionarray.py	`89.14% <54.54%> (-0.70%)`	⬇️
src/awkward/_v2/highlevel.py	`70.77% <57.60%> (-2.76%)`	⬇️
src/awkward/_v2/operations/ak_from_parquet.py	`87.02% <88.09%> (+37.87%)`	⬆️
src/awkward/_v2/operations/ak_to_parquet.py	`51.02% <95.23%> (+3.02%)`	⬆️
src/awkward/_v2/_connect/numba/arrayview.py	`96.76% <100.00%> (ø)`
src/awkward/_v2/contents/content.py	`76.61% <100.00%> (ø)`
src/awkward/_v2/operations/ak_copy.py	`100.00% <100.00%> (ø)`
src/awkward/_v2/operations/ak_is_none.py	`96.55% <100.00%> (+0.39%)`	⬆️
...awkward/_v2/operations/ak_metadata_from_parquet.py	`100.00% <100.00%> (+68.42%)`	⬆️
src/awkward/_v2/record.py	`77.53% <100.00%> (+0.49%)`	⬆️
... and 21 more

ianna · 2022-07-25T21:07:07Z

@jpivarski - please, check when you have time. Thanks!

…ng std::tuple_element_t on all MacOS CI nodes

for more information, see https://pre-commit.ci

ianna · 2022-07-27T14:40:37Z

@jpivarski - this PR is complete. Please, check when you have time. Thanks.

jpivarski

As far as I can see, this PR does a few things:

It passes BuilderOptions through a lot of constructors, rather than a single integer (initial). That will allow us to expand the options, such as a distinction between the initial GrowableBuffer panel size and subsequent ones. (We'll want to make those arguments not template parameters in LayoutBuilder, since I doubt an object like BuilderOptions can be a compile-time constant. ~~If LayoutBuilder needs to have a default constructor, maybe the default constructor can supply default BuilderOptions?)~~ I see that you did that already.
Various clean-ups, which cause indentation diffs.

On closer inspection, it looks to me like the only substantial change is that initial → options, with all the supporting changes that needs. Is this right? (Have I missed anything?)

The only thing I would say needs to be changed is that a C++ user sees these options with a class name like awkward::BuilderOptions, not awkward::Options, because these options are specifically for builders.

src/awkward/_v2/cpp-headers/awkward/GrowableBuffer.h

tests-cpp/test_1560-builder-options.cpp

ianna · 2022-07-28T14:41:56Z

As far as I can see, this PR does a few things:

It passes BuilderOptions through a lot of constructors, rather than a single integer (initial). That will allow us to expand the options, such as a distinction between the initial GrowableBuffer panel size and subsequent ones. (We'll want to make those arguments not template parameters in LayoutBuilder, since I doubt an object like BuilderOptions can be a compile-time constant. ~~If LayoutBuilder needs to have a default constructor, maybe the default constructor can supply default BuilderOptions?)~~ I see that you did that already.

Various clean-ups, which cause indentation diffs.

On closer inspection, it looks to me like the only substantial change is that initial → options, with all the supporting changes that needs. Is this right? (Have I missed anything?)

just small stuff:

std::void_t has been replaced as it is C++17
BitMasked mask was fixed. That required additional GrowableBuffer concatenate method with an offset defined.
cleanup LayoutBuilder API - remove temporary/legacy to_buffer method

The only thing I would say needs to be changed is that a C++ user sees these options with a class name like awkward::BuilderOptions, not awkward::Options, because these options are specifically for builders.

jpivarski

Okay, great—it's ready to merge, then!

* restore array builder options * GrowableBuffer and its test updated * builder options added * introduce builder options to layout builder * drop C++17 requirement for cpp-only tests to C++14; work-around missing std::tuple_element_t on all MacOS CI nodes * fix compilation error and cleanup * clang format * remove default parameters from build options * remove initializer list * fix bitmasked mask and add builder options test * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * address Jim's comments Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

ianna marked this pull request as draft July 22, 2022 15:49

ianna requested a review from jpivarski July 25, 2022 21:06

ianna marked this pull request as ready for review July 25, 2022 21:06

ianna mentioned this pull request Jul 25, 2022

Templated LayoutBuilder #1494

Merged

15 tasks

ianna marked this pull request as draft July 26, 2022 07:01

ianna added 3 commits July 26, 2022 09:13

restore array builder options

1c26cc5

GrowableBuffer and its test updated

6bf6f6c

builder options added

3d16a35

ianna force-pushed the ianna/array-builder-options branch from 647d0d2 to 3d16a35 Compare July 26, 2022 07:19

introduce builder options to layout builder

7f0d25a

ianna mentioned this pull request Jul 26, 2022

Massive memory leak with awkward.Array #1127

Closed

ianna added 5 commits July 26, 2022 16:44

drop C++17 requirement for cpp-only tests to C++14; work-around missi…

1b51954

…ng std::tuple_element_t on all MacOS CI nodes

fix compilation error and cleanup

4feafcb

clang format

814b3c5

remove default parameters from build options

48c3e08

remove initializer list

9c00e1a

ianna marked this pull request as ready for review July 26, 2022 16:35

ianna and others added 2 commits July 27, 2022 09:01

fix bitmasked mask and add builder options test

5c4f92b

[pre-commit.ci] auto fixes from pre-commit.com hooks

9dd794f

for more information, see https://pre-commit.ci

ianna changed the title ~~restore array builder options~~ builder options and layout builder updates Jul 27, 2022

ianna mentioned this pull request Jul 27, 2022

Growable Buffer for ForthMachine #1517

Closed

jpivarski reviewed Jul 28, 2022

View reviewed changes

src/awkward/_v2/cpp-headers/awkward/GrowableBuffer.h Show resolved Hide resolved

src/awkward/_v2/cpp-headers/awkward/GrowableBuffer.h Outdated Show resolved Hide resolved

tests-cpp/test_1560-builder-options.cpp Outdated Show resolved Hide resolved

address Jim's comments

65a73ee

ianna requested a review from jpivarski July 28, 2022 15:03

jpivarski approved these changes Jul 28, 2022

View reviewed changes

jpivarski merged commit 28218f3 into main Jul 28, 2022

jpivarski deleted the ianna/array-builder-options branch July 28, 2022 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

builder options and layout builder updates #1560

builder options and layout builder updates #1560

ianna commented Jul 22, 2022 •

edited

Loading

codecov bot commented Jul 22, 2022 •

edited

Loading

ianna commented Jul 25, 2022

ianna commented Jul 27, 2022

jpivarski left a comment

ianna commented Jul 28, 2022

jpivarski left a comment

builder options and layout builder updates #1560

builder options and layout builder updates #1560

Conversation

ianna commented Jul 22, 2022 • edited Loading

codecov bot commented Jul 22, 2022 • edited Loading

Codecov Report

ianna commented Jul 25, 2022

ianna commented Jul 27, 2022

jpivarski left a comment

Choose a reason for hiding this comment

ianna commented Jul 28, 2022

jpivarski left a comment

Choose a reason for hiding this comment

ianna commented Jul 22, 2022 •

edited

Loading

codecov bot commented Jul 22, 2022 •

edited

Loading