ARROW-6077: [C++][Parquet] Build Arrow "schema tree" from Parquet schema to help with nested data implementation

[wesm]

Introduces auxiliary internal SchemaManifest and SchemaField data structures.

This also permits dictionary-encoded subfields in a slightly more principled way (the dictionary type creation is resolved one time, so this removes the FixSchema hacks that were there before). I rewrote the nested schema conversion logic to hopefully be slightly easier to follow though it could still use some work. I added comments within to explain the 3 different styles of list encoding

There are a couple of API changes:

• The FileReader::GetSchema(indices, &schema) method has been removed. The way that "projected" schemas were being constructed was pretty hacky, and this function is non-essential to the operation of the class. I had to remove bindings in the GLib and R libraries for this function, but as far as I can tell these bindings were non-essential to operation, and were added only because the function was there to wrap.
• Added FileWriter::Make factory method, making constructor private

This patch was pretty unpleasant to do -- it removes some hacky functions used to create Arrow fields with leaf nodes trimmed. There is little functional change; it is an attempt to bring a cleaner structure for full-fledged nested data reading

I'm going to get on with seeing through user-facing dictionary-encoding functionality in Python

[codecov-io]

Codecov Report

Merging #4971 into master will increase coverage by 1.6%.
The diff coverage is 91.34%.

@@ Coverage Diff @@## master #4971 +/- ##
==========================================
+ Coverage 87.55% 89.15% +1.6% 
==========================================
Files 1000 723 -277 Lines 142303 101705 -40598 Branches 1418 0 -1418 ==========================================
- Hits 124589 90678 -33911 + Misses 17352 11027 -6325 + Partials 362 0 -362

Impacted Files	Coverage Δ
cpp/src/parquet/arrow/writer.h	100% <ø> (ø)	⬆️
cpp/src/parquet/arrow/schema.cc	93.21% <ø> (+2.24%)	⬆️
cpp/src/parquet/arrow/reader.h	91.66% <ø> (ø)	⬆️
cpp/src/parquet/schema.h	100% <ø> (ø)	⬆️
cpp/src/parquet/schema-internal.h	100% <ø> (+3.44%)	⬆️
r/R/arrowExports.R	71.12% <100%> (+0.29%)	⬆️
cpp/src/parquet/file_writer.cc	81.21% <100%> (-1.02%)	⬇️
r/src/arrowExports.cpp	70.91% <100%> (+0.33%)	⬆️
r/src/parquet.cpp	70.58% <100%> (+7.43%)	⬆️
r/R/parquet.R	72% <50%> (+5.33%)	⬆️
... and 305 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 803dd89...a5187c2. Read the comment docs.

[wesm]

Looks like I have to rebase. Any thoughts about this? I need to stack my patches on top

[fsaintjacques]

@wesm I have a pending review I started yesterday night, let me complete this morning (lot to review!)

[wesm]

Okay. Quite a bit code has been moved around relatively unmodified so please refrain from commenting on old code. Thanks =)

[fsaintjacques]

I'll try to, github review interface does not make this easy to do :)

[hatemhelal]

I had a read through and looked good to me. I think this or your previous refactoring PR might have fixed ARROW-6005. I'll plan to use that issue to add some unittests for that particular workflow.

[hatemhelal]

I found this change from passing by const ref to value threw me a bit. Could warrant a comment? Or does it make sense to keep the signature as accepting a const ref and move this:

::arrow::dictionary(::arrow::int32(), logical_value_type)

into TransferDictionary?

[wesm]

I'm reverting this change, it was a leftover from mid-way through the patch

[hatemhelal]

I think this code was moved from elsewhere but you could replace this with boost::algorithm::ends_with

[wesm]

Cool, doing that then =)

[bkietz]

Suggested change

	std::vector<int> Arange(int length) {
	staticstd::vector<int> Arange(int length) {

[wesm]

Moved to anonymous namespace

[bkietz]

Suggested change

	explicitRowGroupRecordBatchReader(
	RowGroupRecordBatchReader(

[wesm]

done

[bkietz]

This seems like a misleading error message since non-leaf schema fields don't have indices

[wesm]

Tricky thing to have any kind of error here, since users should not see this. I am changing to say that the schema manifest may be malformed

[bkietz]

Could you elaborate? Is this in reference to #4956 ?

[wesm]

The problem that I'm referencing here is fixed by this patch, I will remove the comment

[wesm]

I opened https://issues.apache.org/jira/browse/ARROW-6105 so this gets tested

[bkietz]

Suggested change

	}else {
	}

[wesm]

This is old code

[bkietz]

Suggested change

	if (((empty_def_level[j] - 1) == def_levels[i]) && (nullable[j])) {
	if (nullable[j] && empty_def_level[j] - 1 == def_levels[i]) {

[wesm]

Old code

[bkietz]

Might be easier than offset_builders and valid_bits_builders:

MakeBuilder(pool, list(list(list(/*...*/null()/*...*/))), &root_builder);

[wesm]

Old code

[wesm]

Thank you for the feedback. I'll merge this once the build passes

[wesm]

Travis CI: https://travis-ci.org/wesm/arrow/builds/566680039
Appveyor: https://ci.appveyor.com/project/wesm/arrow/builds/26412833

The Appveyor build isn't done yet so I'll wait a little bit to make sure MinGW is ok

[wesm]

One of the Windows builds is hanging, it doesn't appear related to this patch because I found it in a build in master. I opened

https://issues.apache.org/jira/browse/ARROW-6108

Merging this in the meantime

[wesm]

Introduces auxiliary internal SchemaManifest and SchemaField data structures.

This also permits dictionary-encoded subfields in a slightly more principled way (the dictionary type creation is resolved one time, so this removes the FixSchema hacks that were there before). I rewrote the nested schema conversion logic to hopefully be slightly easier to follow though it could still use some work. I added comments within to explain the 3 different styles of list encoding

There are a couple of API changes:

• The FileReader::GetSchema(indices, &schema) method has been removed. The way that "projected" schemas were being constructed was pretty hacky, and this function is non-essential to the operation of the class. I had to remove bindings in the GLib and R libraries for this function, but as far as I can tell these bindings were non-essential to operation, and were added only because the function was there to wrap.
• Added FileWriter::Make factory method, making constructor private

This patch was pretty unpleasant to do -- it removes some hacky functions used to create Arrow fields with leaf nodes trimmed. There is little functional change; it is an attempt to bring a cleaner structure for full-fledged nested data reading

I'm going to get on with seeing through user-facing dictionary-encoding functionality in Python

[codecov-io]

Codecov Report

Merging #4971 into master will increase coverage by 1.6%.
The diff coverage is 91.34%.

@@ Coverage Diff @@## master #4971 +/- ##
==========================================
+ Coverage 87.55% 89.15% +1.6% 
==========================================
Files 1000 723 -277 Lines 142303 101705 -40598 Branches 1418 0 -1418 ==========================================
- Hits 124589 90678 -33911 + Misses 17352 11027 -6325 + Partials 362 0 -362

Impacted Files	Coverage Δ
cpp/src/parquet/arrow/writer.h	100% <ø> (ø)	⬆️
cpp/src/parquet/arrow/schema.cc	93.21% <ø> (+2.24%)	⬆️
cpp/src/parquet/arrow/reader.h	91.66% <ø> (ø)	⬆️
cpp/src/parquet/schema.h	100% <ø> (ø)	⬆️
cpp/src/parquet/schema-internal.h	100% <ø> (+3.44%)	⬆️
r/R/arrowExports.R	71.12% <100%> (+0.29%)	⬆️
cpp/src/parquet/file_writer.cc	81.21% <100%> (-1.02%)	⬇️
r/src/arrowExports.cpp	70.91% <100%> (+0.33%)	⬆️
r/src/parquet.cpp	70.58% <100%> (+7.43%)	⬆️
r/R/parquet.R	72% <50%> (+5.33%)	⬆️
... and 305 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 803dd89...a5187c2. Read the comment docs.

[wesm]

Looks like I have to rebase. Any thoughts about this? I need to stack my patches on top

[fsaintjacques]

@wesm I have a pending review I started yesterday night, let me complete this morning (lot to review!)

[wesm]

Okay. Quite a bit code has been moved around relatively unmodified so please refrain from commenting on old code. Thanks =)

[fsaintjacques]

I'll try to, github review interface does not make this easy to do :)

[hatemhelal]

I had a read through and looked good to me. I think this or your previous refactoring PR might have fixed ARROW-6005. I'll plan to use that issue to add some unittests for that particular workflow.

[hatemhelal]

I found this change from passing by const ref to value threw me a bit. Could warrant a comment? Or does it make sense to keep the signature as accepting a const ref and move this:

::arrow::dictionary(::arrow::int32(), logical_value_type)

into TransferDictionary?

[wesm]

I'm reverting this change, it was a leftover from mid-way through the patch

[hatemhelal]

I think this code was moved from elsewhere but you could replace this with boost::algorithm::ends_with

[wesm]

Cool, doing that then =)

[bkietz]

Suggested change

	std::vector<int> Arange(int length) {
	staticstd::vector<int> Arange(int length) {

[wesm]

Moved to anonymous namespace

[bkietz]

Suggested change

	explicitRowGroupRecordBatchReader(
	RowGroupRecordBatchReader(

[wesm]

done

[bkietz]

This seems like a misleading error message since non-leaf schema fields don't have indices

[wesm]

Tricky thing to have any kind of error here, since users should not see this. I am changing to say that the schema manifest may be malformed

[bkietz]

Could you elaborate? Is this in reference to #4956 ?

[wesm]

The problem that I'm referencing here is fixed by this patch, I will remove the comment

[wesm]

I opened https://issues.apache.org/jira/browse/ARROW-6105 so this gets tested

[bkietz]

Suggested change

	}else {
	}

[wesm]

This is old code

[bkietz]

Suggested change

	if (((empty_def_level[j] - 1) == def_levels[i]) && (nullable[j])) {
	if (nullable[j] && empty_def_level[j] - 1 == def_levels[i]) {

[wesm]

Old code

[bkietz]

Might be easier than offset_builders and valid_bits_builders:

MakeBuilder(pool, list(list(list(/*...*/null()/*...*/))), &root_builder);

[wesm]

Old code

[wesm]

Thank you for the feedback. I'll merge this once the build passes

[wesm]

Travis CI: https://travis-ci.org/wesm/arrow/builds/566680039
Appveyor: https://ci.appveyor.com/project/wesm/arrow/builds/26412833

The Appveyor build isn't done yet so I'll wait a little bit to make sure MinGW is ok

[wesm]

One of the Windows builds is hanging, it doesn't appear related to this patch because I found it in a build in master. I opened

https://issues.apache.org/jira/browse/ARROW-6108

Merging this in the meantime

[wesm]

Introduces auxiliary internal SchemaManifest and SchemaField data structures.

This also permits dictionary-encoded subfields in a slightly more principled way (the dictionary type creation is resolved one time, so this removes the FixSchema hacks that were there before). I rewrote the nested schema conversion logic to hopefully be slightly easier to follow though it could still use some work. I added comments within to explain the 3 different styles of list encoding

There are a couple of API changes:

• The FileReader::GetSchema(indices, &schema) method has been removed. The way that "projected" schemas were being constructed was pretty hacky, and this function is non-essential to the operation of the class. I had to remove bindings in the GLib and R libraries for this function, but as far as I can tell these bindings were non-essential to operation, and were added only because the function was there to wrap.
• Added FileWriter::Make factory method, making constructor private

This patch was pretty unpleasant to do -- it removes some hacky functions used to create Arrow fields with leaf nodes trimmed. There is little functional change; it is an attempt to bring a cleaner structure for full-fledged nested data reading

I'm going to get on with seeing through user-facing dictionary-encoding functionality in Python

[codecov-io]

Codecov Report

Merging #4971 into master will increase coverage by 1.6%.
The diff coverage is 91.34%.

@@ Coverage Diff @@## master #4971 +/- ##
==========================================
+ Coverage 87.55% 89.15% +1.6% 
==========================================
Files 1000 723 -277 Lines 142303 101705 -40598 Branches 1418 0 -1418 ==========================================
- Hits 124589 90678 -33911 + Misses 17352 11027 -6325 + Partials 362 0 -362

Impacted Files	Coverage Δ
cpp/src/parquet/arrow/writer.h	100% <ø> (ø)	⬆️
cpp/src/parquet/arrow/schema.cc	93.21% <ø> (+2.24%)	⬆️
cpp/src/parquet/arrow/reader.h	91.66% <ø> (ø)	⬆️
cpp/src/parquet/schema.h	100% <ø> (ø)	⬆️
cpp/src/parquet/schema-internal.h	100% <ø> (+3.44%)	⬆️
r/R/arrowExports.R	71.12% <100%> (+0.29%)	⬆️
cpp/src/parquet/file_writer.cc	81.21% <100%> (-1.02%)	⬇️
r/src/arrowExports.cpp	70.91% <100%> (+0.33%)	⬆️
r/src/parquet.cpp	70.58% <100%> (+7.43%)	⬆️
r/R/parquet.R	72% <50%> (+5.33%)	⬆️
... and 305 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 803dd89...a5187c2. Read the comment docs.

[wesm]

Looks like I have to rebase. Any thoughts about this? I need to stack my patches on top

[fsaintjacques]

@wesm I have a pending review I started yesterday night, let me complete this morning (lot to review!)

[wesm]

Okay. Quite a bit code has been moved around relatively unmodified so please refrain from commenting on old code. Thanks =)

[fsaintjacques]

I'll try to, github review interface does not make this easy to do :)

[hatemhelal]

I had a read through and looked good to me. I think this or your previous refactoring PR might have fixed ARROW-6005. I'll plan to use that issue to add some unittests for that particular workflow.

[hatemhelal]

I found this change from passing by const ref to value threw me a bit. Could warrant a comment? Or does it make sense to keep the signature as accepting a const ref and move this:

::arrow::dictionary(::arrow::int32(), logical_value_type)

into TransferDictionary?

[wesm]

I'm reverting this change, it was a leftover from mid-way through the patch

[hatemhelal]

I think this code was moved from elsewhere but you could replace this with boost::algorithm::ends_with

[wesm]

Cool, doing that then =)

[bkietz]

Suggested change

	std::vector<int> Arange(int length) {
	staticstd::vector<int> Arange(int length) {

[wesm]

Moved to anonymous namespace

[bkietz]

Suggested change

	explicitRowGroupRecordBatchReader(
	RowGroupRecordBatchReader(

[wesm]

done

[bkietz]

This seems like a misleading error message since non-leaf schema fields don't have indices

[wesm]

Tricky thing to have any kind of error here, since users should not see this. I am changing to say that the schema manifest may be malformed

[bkietz]

Could you elaborate? Is this in reference to #4956 ?

[wesm]

The problem that I'm referencing here is fixed by this patch, I will remove the comment

[wesm]

I opened https://issues.apache.org/jira/browse/ARROW-6105 so this gets tested

[bkietz]

Suggested change

	}else {
	}

[wesm]

This is old code

[bkietz]

Suggested change

	if (((empty_def_level[j] - 1) == def_levels[i]) && (nullable[j])) {
	if (nullable[j] && empty_def_level[j] - 1 == def_levels[i]) {

[wesm]

Old code

[bkietz]

Might be easier than offset_builders and valid_bits_builders:

MakeBuilder(pool, list(list(list(/*...*/null()/*...*/))), &root_builder);

[wesm]

Old code

[wesm]

Thank you for the feedback. I'll merge this once the build passes

[wesm]

Travis CI: https://travis-ci.org/wesm/arrow/builds/566680039
Appveyor: https://ci.appveyor.com/project/wesm/arrow/builds/26412833

The Appveyor build isn't done yet so I'll wait a little bit to make sure MinGW is ok

[wesm]

One of the Windows builds is hanging, it doesn't appear related to this patch because I found it in a build in master. I opened

https://issues.apache.org/jira/browse/ARROW-6108

Merging this in the meantime

[wesm]

Introduces auxiliary internal SchemaManifest and SchemaField data structures.

This also permits dictionary-encoded subfields in a slightly more principled way (the dictionary type creation is resolved one time, so this removes the FixSchema hacks that were there before). I rewrote the nested schema conversion logic to hopefully be slightly easier to follow though it could still use some work. I added comments within to explain the 3 different styles of list encoding

There are a couple of API changes:

• The FileReader::GetSchema(indices, &schema) method has been removed. The way that "projected" schemas were being constructed was pretty hacky, and this function is non-essential to the operation of the class. I had to remove bindings in the GLib and R libraries for this function, but as far as I can tell these bindings were non-essential to operation, and were added only because the function was there to wrap.
• Added FileWriter::Make factory method, making constructor private

This patch was pretty unpleasant to do -- it removes some hacky functions used to create Arrow fields with leaf nodes trimmed. There is little functional change; it is an attempt to bring a cleaner structure for full-fledged nested data reading

I'm going to get on with seeing through user-facing dictionary-encoding functionality in Python

[codecov-io]

Codecov Report

Merging #4971 into master will increase coverage by 1.6%.
The diff coverage is 91.34%.

@@ Coverage Diff @@## master #4971 +/- ##
==========================================
+ Coverage 87.55% 89.15% +1.6% 
==========================================
Files 1000 723 -277 Lines 142303 101705 -40598 Branches 1418 0 -1418 ==========================================
- Hits 124589 90678 -33911 + Misses 17352 11027 -6325 + Partials 362 0 -362

Impacted Files	Coverage Δ
cpp/src/parquet/arrow/writer.h	100% <ø> (ø)	⬆️
cpp/src/parquet/arrow/schema.cc	93.21% <ø> (+2.24%)	⬆️
cpp/src/parquet/arrow/reader.h	91.66% <ø> (ø)	⬆️
cpp/src/parquet/schema.h	100% <ø> (ø)	⬆️
cpp/src/parquet/schema-internal.h	100% <ø> (+3.44%)	⬆️
r/R/arrowExports.R	71.12% <100%> (+0.29%)	⬆️
cpp/src/parquet/file_writer.cc	81.21% <100%> (-1.02%)	⬇️
r/src/arrowExports.cpp	70.91% <100%> (+0.33%)	⬆️
r/src/parquet.cpp	70.58% <100%> (+7.43%)	⬆️
r/R/parquet.R	72% <50%> (+5.33%)	⬆️
... and 305 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 803dd89...a5187c2. Read the comment docs.

[wesm]

Looks like I have to rebase. Any thoughts about this? I need to stack my patches on top

[fsaintjacques]

@wesm I have a pending review I started yesterday night, let me complete this morning (lot to review!)

[wesm]

Okay. Quite a bit code has been moved around relatively unmodified so please refrain from commenting on old code. Thanks =)

[fsaintjacques]

I'll try to, github review interface does not make this easy to do :)

[hatemhelal]

I had a read through and looked good to me. I think this or your previous refactoring PR might have fixed ARROW-6005. I'll plan to use that issue to add some unittests for that particular workflow.

[hatemhelal]

I found this change from passing by const ref to value threw me a bit. Could warrant a comment? Or does it make sense to keep the signature as accepting a const ref and move this:

::arrow::dictionary(::arrow::int32(), logical_value_type)

into TransferDictionary?

[wesm]

I'm reverting this change, it was a leftover from mid-way through the patch

[hatemhelal]

I think this code was moved from elsewhere but you could replace this with boost::algorithm::ends_with

[wesm]

Cool, doing that then =)

[bkietz]

Suggested change

	std::vector<int> Arange(int length) {
	staticstd::vector<int> Arange(int length) {

[wesm]

Moved to anonymous namespace

[bkietz]

Suggested change

	explicitRowGroupRecordBatchReader(
	RowGroupRecordBatchReader(

[wesm]

done

[bkietz]

This seems like a misleading error message since non-leaf schema fields don't have indices

[wesm]

Tricky thing to have any kind of error here, since users should not see this. I am changing to say that the schema manifest may be malformed

[bkietz]

Could you elaborate? Is this in reference to #4956 ?

[wesm]

The problem that I'm referencing here is fixed by this patch, I will remove the comment

[wesm]

I opened https://issues.apache.org/jira/browse/ARROW-6105 so this gets tested

[bkietz]

Suggested change

	}else {
	}

[wesm]

This is old code

[bkietz]

Suggested change

	if (((empty_def_level[j] - 1) == def_levels[i]) && (nullable[j])) {
	if (nullable[j] && empty_def_level[j] - 1 == def_levels[i]) {

[wesm]

Old code

[bkietz]

Might be easier than offset_builders and valid_bits_builders:

MakeBuilder(pool, list(list(list(/*...*/null()/*...*/))), &root_builder);

[wesm]

Old code

[wesm]

Thank you for the feedback. I'll merge this once the build passes

[wesm]

Travis CI: https://travis-ci.org/wesm/arrow/builds/566680039
Appveyor: https://ci.appveyor.com/project/wesm/arrow/builds/26412833

The Appveyor build isn't done yet so I'll wait a little bit to make sure MinGW is ok

[wesm]

One of the Windows builds is hanging, it doesn't appear related to this patch because I found it in a build in master. I opened

https://issues.apache.org/jira/browse/ARROW-6108

Merging this in the meantime

[wesm]

Introduces auxiliary internal SchemaManifest and SchemaField data structures.

This also permits dictionary-encoded subfields in a slightly more principled way (the dictionary type creation is resolved one time, so this removes the FixSchema hacks that were there before). I rewrote the nested schema conversion logic to hopefully be slightly easier to follow though it could still use some work. I added comments within to explain the 3 different styles of list encoding

There are a couple of API changes:

• The FileReader::GetSchema(indices, &schema) method has been removed. The way that "projected" schemas were being constructed was pretty hacky, and this function is non-essential to the operation of the class. I had to remove bindings in the GLib and R libraries for this function, but as far as I can tell these bindings were non-essential to operation, and were added only because the function was there to wrap.
• Added FileWriter::Make factory method, making constructor private

This patch was pretty unpleasant to do -- it removes some hacky functions used to create Arrow fields with leaf nodes trimmed. There is little functional change; it is an attempt to bring a cleaner structure for full-fledged nested data reading

I'm going to get on with seeing through user-facing dictionary-encoding functionality in Python

[codecov-io]

Codecov Report

Merging #4971 into master will increase coverage by 1.6%.
The diff coverage is 91.34%.

@@ Coverage Diff @@## master #4971 +/- ##
==========================================
+ Coverage 87.55% 89.15% +1.6% 
==========================================
Files 1000 723 -277 Lines 142303 101705 -40598 Branches 1418 0 -1418 ==========================================
- Hits 124589 90678 -33911 + Misses 17352 11027 -6325 + Partials 362 0 -362

Impacted Files	Coverage Δ
cpp/src/parquet/arrow/writer.h	100% <ø> (ø)	⬆️
cpp/src/parquet/arrow/schema.cc	93.21% <ø> (+2.24%)	⬆️
cpp/src/parquet/arrow/reader.h	91.66% <ø> (ø)	⬆️
cpp/src/parquet/schema.h	100% <ø> (ø)	⬆️
cpp/src/parquet/schema-internal.h	100% <ø> (+3.44%)	⬆️
r/R/arrowExports.R	71.12% <100%> (+0.29%)	⬆️
cpp/src/parquet/file_writer.cc	81.21% <100%> (-1.02%)	⬇️
r/src/arrowExports.cpp	70.91% <100%> (+0.33%)	⬆️
r/src/parquet.cpp	70.58% <100%> (+7.43%)	⬆️
r/R/parquet.R	72% <50%> (+5.33%)	⬆️
... and 305 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 803dd89...a5187c2. Read the comment docs.

[wesm]

Looks like I have to rebase. Any thoughts about this? I need to stack my patches on top

[fsaintjacques]

@wesm I have a pending review I started yesterday night, let me complete this morning (lot to review!)

[wesm]

Okay. Quite a bit code has been moved around relatively unmodified so please refrain from commenting on old code. Thanks =)

[fsaintjacques]

I'll try to, github review interface does not make this easy to do :)

[hatemhelal]

I had a read through and looked good to me. I think this or your previous refactoring PR might have fixed ARROW-6005. I'll plan to use that issue to add some unittests for that particular workflow.

[hatemhelal]

I found this change from passing by const ref to value threw me a bit. Could warrant a comment? Or does it make sense to keep the signature as accepting a const ref and move this:

::arrow::dictionary(::arrow::int32(), logical_value_type)

into TransferDictionary?

[wesm]

I'm reverting this change, it was a leftover from mid-way through the patch

[hatemhelal]

I think this code was moved from elsewhere but you could replace this with boost::algorithm::ends_with

[wesm]

Cool, doing that then =)

[bkietz]

Suggested change

	std::vector<int> Arange(int length) {
	staticstd::vector<int> Arange(int length) {

[wesm]

Moved to anonymous namespace

[bkietz]

Suggested change

	explicitRowGroupRecordBatchReader(
	RowGroupRecordBatchReader(

[wesm]

done

[bkietz]

This seems like a misleading error message since non-leaf schema fields don't have indices

[wesm]

Tricky thing to have any kind of error here, since users should not see this. I am changing to say that the schema manifest may be malformed

[bkietz]

Could you elaborate? Is this in reference to #4956 ?

[wesm]

The problem that I'm referencing here is fixed by this patch, I will remove the comment

[wesm]

I opened https://issues.apache.org/jira/browse/ARROW-6105 so this gets tested

[bkietz]

Suggested change

	}else {
	}

[wesm]

This is old code

[bkietz]

Suggested change

	if (((empty_def_level[j] - 1) == def_levels[i]) && (nullable[j])) {
	if (nullable[j] && empty_def_level[j] - 1 == def_levels[i]) {

[wesm]

Old code

[bkietz]

Might be easier than offset_builders and valid_bits_builders:

MakeBuilder(pool, list(list(list(/*...*/null()/*...*/))), &root_builder);

[wesm]

Old code

[wesm]

Thank you for the feedback. I'll merge this once the build passes

[wesm]

Travis CI: https://travis-ci.org/wesm/arrow/builds/566680039
Appveyor: https://ci.appveyor.com/project/wesm/arrow/builds/26412833

The Appveyor build isn't done yet so I'll wait a little bit to make sure MinGW is ok

[wesm]

One of the Windows builds is hanging, it doesn't appear related to this patch because I found it in a build in master. I opened

https://issues.apache.org/jira/browse/ARROW-6108

Merging this in the meantime

[wesm]

Introduces auxiliary internal SchemaManifest and SchemaField data structures.

This also permits dictionary-encoded subfields in a slightly more principled way (the dictionary type creation is resolved one time, so this removes the FixSchema hacks that were there before). I rewrote the nested schema conversion logic to hopefully be slightly easier to follow though it could still use some work. I added comments within to explain the 3 different styles of list encoding

There are a couple of API changes:

• The FileReader::GetSchema(indices, &schema) method has been removed. The way that "projected" schemas were being constructed was pretty hacky, and this function is non-essential to the operation of the class. I had to remove bindings in the GLib and R libraries for this function, but as far as I can tell these bindings were non-essential to operation, and were added only because the function was there to wrap.
• Added FileWriter::Make factory method, making constructor private

This patch was pretty unpleasant to do -- it removes some hacky functions used to create Arrow fields with leaf nodes trimmed. There is little functional change; it is an attempt to bring a cleaner structure for full-fledged nested data reading

I'm going to get on with seeing through user-facing dictionary-encoding functionality in Python

[codecov-io]

Codecov Report

Merging #4971 into master will increase coverage by 1.6%.
The diff coverage is 91.34%.

@@ Coverage Diff @@## master #4971 +/- ##
==========================================
+ Coverage 87.55% 89.15% +1.6% 
==========================================
Files 1000 723 -277 Lines 142303 101705 -40598 Branches 1418 0 -1418 ==========================================
- Hits 124589 90678 -33911 + Misses 17352 11027 -6325 + Partials 362 0 -362

Impacted Files	Coverage Δ
cpp/src/parquet/arrow/writer.h	100% <ø> (ø)	⬆️
cpp/src/parquet/arrow/schema.cc	93.21% <ø> (+2.24%)	⬆️
cpp/src/parquet/arrow/reader.h	91.66% <ø> (ø)	⬆️
cpp/src/parquet/schema.h	100% <ø> (ø)	⬆️
cpp/src/parquet/schema-internal.h	100% <ø> (+3.44%)	⬆️
r/R/arrowExports.R	71.12% <100%> (+0.29%)	⬆️
cpp/src/parquet/file_writer.cc	81.21% <100%> (-1.02%)	⬇️
r/src/arrowExports.cpp	70.91% <100%> (+0.33%)	⬆️
r/src/parquet.cpp	70.58% <100%> (+7.43%)	⬆️
r/R/parquet.R	72% <50%> (+5.33%)	⬆️
... and 305 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 803dd89...a5187c2. Read the comment docs.

[wesm]

Looks like I have to rebase. Any thoughts about this? I need to stack my patches on top

[fsaintjacques]

@wesm I have a pending review I started yesterday night, let me complete this morning (lot to review!)

[wesm]

Okay. Quite a bit code has been moved around relatively unmodified so please refrain from commenting on old code. Thanks =)

[fsaintjacques]

I'll try to, github review interface does not make this easy to do :)

[hatemhelal]

I had a read through and looked good to me. I think this or your previous refactoring PR might have fixed ARROW-6005. I'll plan to use that issue to add some unittests for that particular workflow.

[hatemhelal]

I found this change from passing by const ref to value threw me a bit. Could warrant a comment? Or does it make sense to keep the signature as accepting a const ref and move this:

::arrow::dictionary(::arrow::int32(), logical_value_type)

into TransferDictionary?

[wesm]

I'm reverting this change, it was a leftover from mid-way through the patch

[hatemhelal]

I think this code was moved from elsewhere but you could replace this with boost::algorithm::ends_with

[wesm]

Cool, doing that then =)

[bkietz]

Suggested change

	std::vector<int> Arange(int length) {
	staticstd::vector<int> Arange(int length) {

[wesm]

Moved to anonymous namespace

[bkietz]

Suggested change

	explicitRowGroupRecordBatchReader(
	RowGroupRecordBatchReader(

[wesm]

done

[bkietz]

This seems like a misleading error message since non-leaf schema fields don't have indices

[wesm]

Tricky thing to have any kind of error here, since users should not see this. I am changing to say that the schema manifest may be malformed

[bkietz]

Could you elaborate? Is this in reference to #4956 ?

[wesm]

The problem that I'm referencing here is fixed by this patch, I will remove the comment

[wesm]

I opened https://issues.apache.org/jira/browse/ARROW-6105 so this gets tested

[bkietz]

Suggested change

	}else {
	}

[wesm]

This is old code

[bkietz]

Suggested change

	if (((empty_def_level[j] - 1) == def_levels[i]) && (nullable[j])) {
	if (nullable[j] && empty_def_level[j] - 1 == def_levels[i]) {

[wesm]

Old code

[bkietz]

Might be easier than offset_builders and valid_bits_builders:

MakeBuilder(pool, list(list(list(/*...*/null()/*...*/))), &root_builder);

[wesm]

Old code

[wesm]

Thank you for the feedback. I'll merge this once the build passes

[wesm]

Travis CI: https://travis-ci.org/wesm/arrow/builds/566680039
Appveyor: https://ci.appveyor.com/project/wesm/arrow/builds/26412833

The Appveyor build isn't done yet so I'll wait a little bit to make sure MinGW is ok

[wesm]

One of the Windows builds is hanging, it doesn't appear related to this patch because I found it in a build in master. I opened

https://issues.apache.org/jira/browse/ARROW-6108

Merging this in the meantime

[wesm]

Introduces auxiliary internal SchemaManifest and SchemaField data structures.

This also permits dictionary-encoded subfields in a slightly more principled way (the dictionary type creation is resolved one time, so this removes the FixSchema hacks that were there before). I rewrote the nested schema conversion logic to hopefully be slightly easier to follow though it could still use some work. I added comments within to explain the 3 different styles of list encoding

There are a couple of API changes:

• The FileReader::GetSchema(indices, &schema) method has been removed. The way that "projected" schemas were being constructed was pretty hacky, and this function is non-essential to the operation of the class. I had to remove bindings in the GLib and R libraries for this function, but as far as I can tell these bindings were non-essential to operation, and were added only because the function was there to wrap.
• Added FileWriter::Make factory method, making constructor private

This patch was pretty unpleasant to do -- it removes some hacky functions used to create Arrow fields with leaf nodes trimmed. There is little functional change; it is an attempt to bring a cleaner structure for full-fledged nested data reading

I'm going to get on with seeing through user-facing dictionary-encoding functionality in Python

[codecov-io]

Codecov Report

Merging #4971 into master will increase coverage by 1.6%.
The diff coverage is 91.34%.

@@ Coverage Diff @@## master #4971 +/- ##
==========================================
+ Coverage 87.55% 89.15% +1.6% 
==========================================
Files 1000 723 -277 Lines 142303 101705 -40598 Branches 1418 0 -1418 ==========================================
- Hits 124589 90678 -33911 + Misses 17352 11027 -6325 + Partials 362 0 -362

Impacted Files	Coverage Δ
cpp/src/parquet/arrow/writer.h	100% <ø> (ø)	⬆️
cpp/src/parquet/arrow/schema.cc	93.21% <ø> (+2.24%)	⬆️
cpp/src/parquet/arrow/reader.h	91.66% <ø> (ø)	⬆️
cpp/src/parquet/schema.h	100% <ø> (ø)	⬆️
cpp/src/parquet/schema-internal.h	100% <ø> (+3.44%)	⬆️
r/R/arrowExports.R	71.12% <100%> (+0.29%)	⬆️
cpp/src/parquet/file_writer.cc	81.21% <100%> (-1.02%)	⬇️
r/src/arrowExports.cpp	70.91% <100%> (+0.33%)	⬆️
r/src/parquet.cpp	70.58% <100%> (+7.43%)	⬆️
r/R/parquet.R	72% <50%> (+5.33%)	⬆️
... and 305 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 803dd89...a5187c2. Read the comment docs.

[wesm]

Looks like I have to rebase. Any thoughts about this? I need to stack my patches on top

[fsaintjacques]

@wesm I have a pending review I started yesterday night, let me complete this morning (lot to review!)

[wesm]

Okay. Quite a bit code has been moved around relatively unmodified so please refrain from commenting on old code. Thanks =)

[fsaintjacques]

I'll try to, github review interface does not make this easy to do :)

[hatemhelal]

I had a read through and looked good to me. I think this or your previous refactoring PR might have fixed ARROW-6005. I'll plan to use that issue to add some unittests for that particular workflow.

[hatemhelal]

I found this change from passing by const ref to value threw me a bit. Could warrant a comment? Or does it make sense to keep the signature as accepting a const ref and move this:

::arrow::dictionary(::arrow::int32(), logical_value_type)

into TransferDictionary?

[wesm]

I'm reverting this change, it was a leftover from mid-way through the patch

[hatemhelal]

I think this code was moved from elsewhere but you could replace this with boost::algorithm::ends_with

[wesm]

Cool, doing that then =)

[bkietz]

Suggested change

	std::vector<int> Arange(int length) {
	staticstd::vector<int> Arange(int length) {

[wesm]

Moved to anonymous namespace

[bkietz]

Suggested change

	explicitRowGroupRecordBatchReader(
	RowGroupRecordBatchReader(

[wesm]

done

[bkietz]

This seems like a misleading error message since non-leaf schema fields don't have indices

[wesm]

Tricky thing to have any kind of error here, since users should not see this. I am changing to say that the schema manifest may be malformed

[bkietz]

Could you elaborate? Is this in reference to #4956 ?

[wesm]

The problem that I'm referencing here is fixed by this patch, I will remove the comment

[wesm]

I opened https://issues.apache.org/jira/browse/ARROW-6105 so this gets tested

[bkietz]

Suggested change

	}else {
	}

[wesm]

This is old code

[bkietz]

Suggested change

	if (((empty_def_level[j] - 1) == def_levels[i]) && (nullable[j])) {
	if (nullable[j] && empty_def_level[j] - 1 == def_levels[i]) {

[wesm]

Old code

[bkietz]

Might be easier than offset_builders and valid_bits_builders:

MakeBuilder(pool, list(list(list(/*...*/null()/*...*/))), &root_builder);

[wesm]

Old code

[wesm]

Thank you for the feedback. I'll merge this once the build passes

[wesm]

Travis CI: https://travis-ci.org/wesm/arrow/builds/566680039
Appveyor: https://ci.appveyor.com/project/wesm/arrow/builds/26412833

The Appveyor build isn't done yet so I'll wait a little bit to make sure MinGW is ok

[wesm]

One of the Windows builds is hanging, it doesn't appear related to this patch because I found it in a build in master. I opened

https://issues.apache.org/jira/browse/ARROW-6108

Merging this in the meantime

[wesm]

Introduces auxiliary internal SchemaManifest and SchemaField data structures.

This also permits dictionary-encoded subfields in a slightly more principled way (the dictionary type creation is resolved one time, so this removes the FixSchema hacks that were there before). I rewrote the nested schema conversion logic to hopefully be slightly easier to follow though it could still use some work. I added comments within to explain the 3 different styles of list encoding

There are a couple of API changes:

• The FileReader::GetSchema(indices, &schema) method has been removed. The way that "projected" schemas were being constructed was pretty hacky, and this function is non-essential to the operation of the class. I had to remove bindings in the GLib and R libraries for this function, but as far as I can tell these bindings were non-essential to operation, and were added only because the function was there to wrap.
• Added FileWriter::Make factory method, making constructor private

This patch was pretty unpleasant to do -- it removes some hacky functions used to create Arrow fields with leaf nodes trimmed. There is little functional change; it is an attempt to bring a cleaner structure for full-fledged nested data reading

I'm going to get on with seeing through user-facing dictionary-encoding functionality in Python

[codecov-io]

Codecov Report

Merging #4971 into master will increase coverage by 1.6%.
The diff coverage is 91.34%.

@@ Coverage Diff @@## master #4971 +/- ##
==========================================
+ Coverage 87.55% 89.15% +1.6% 
==========================================
Files 1000 723 -277 Lines 142303 101705 -40598 Branches 1418 0 -1418 ==========================================
- Hits 124589 90678 -33911 + Misses 17352 11027 -6325 + Partials 362 0 -362

Impacted Files	Coverage Δ
cpp/src/parquet/arrow/writer.h	100% <ø> (ø)	⬆️
cpp/src/parquet/arrow/schema.cc	93.21% <ø> (+2.24%)	⬆️
cpp/src/parquet/arrow/reader.h	91.66% <ø> (ø)	⬆️
cpp/src/parquet/schema.h	100% <ø> (ø)	⬆️
cpp/src/parquet/schema-internal.h	100% <ø> (+3.44%)	⬆️
r/R/arrowExports.R	71.12% <100%> (+0.29%)	⬆️
cpp/src/parquet/file_writer.cc	81.21% <100%> (-1.02%)	⬇️
r/src/arrowExports.cpp	70.91% <100%> (+0.33%)	⬆️
r/src/parquet.cpp	70.58% <100%> (+7.43%)	⬆️
r/R/parquet.R	72% <50%> (+5.33%)	⬆️
... and 305 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 803dd89...a5187c2. Read the comment docs.

[wesm]

Looks like I have to rebase. Any thoughts about this? I need to stack my patches on top

[fsaintjacques]

@wesm I have a pending review I started yesterday night, let me complete this morning (lot to review!)

[wesm]

Okay. Quite a bit code has been moved around relatively unmodified so please refrain from commenting on old code. Thanks =)

[fsaintjacques]

I'll try to, github review interface does not make this easy to do :)

[hatemhelal]

I had a read through and looked good to me. I think this or your previous refactoring PR might have fixed ARROW-6005. I'll plan to use that issue to add some unittests for that particular workflow.

[hatemhelal]

I found this change from passing by const ref to value threw me a bit. Could warrant a comment? Or does it make sense to keep the signature as accepting a const ref and move this:

::arrow::dictionary(::arrow::int32(), logical_value_type)

into TransferDictionary?

[wesm]

I'm reverting this change, it was a leftover from mid-way through the patch

[hatemhelal]

I think this code was moved from elsewhere but you could replace this with boost::algorithm::ends_with

[wesm]

Cool, doing that then =)

[bkietz]

Suggested change

	std::vector<int> Arange(int length) {
	staticstd::vector<int> Arange(int length) {

[wesm]

Moved to anonymous namespace

[bkietz]

Suggested change

	explicitRowGroupRecordBatchReader(
	RowGroupRecordBatchReader(

[wesm]

done

[bkietz]

This seems like a misleading error message since non-leaf schema fields don't have indices

[wesm]

Tricky thing to have any kind of error here, since users should not see this. I am changing to say that the schema manifest may be malformed

[bkietz]

Could you elaborate? Is this in reference to #4956 ?

[wesm]

The problem that I'm referencing here is fixed by this patch, I will remove the comment

[wesm]

I opened https://issues.apache.org/jira/browse/ARROW-6105 so this gets tested

[bkietz]

Suggested change

	}else {
	}

[wesm]

This is old code

[bkietz]

Suggested change

	if (((empty_def_level[j] - 1) == def_levels[i]) && (nullable[j])) {
	if (nullable[j] && empty_def_level[j] - 1 == def_levels[i]) {

[wesm]

Old code

[bkietz]

Might be easier than offset_builders and valid_bits_builders:

MakeBuilder(pool, list(list(list(/*...*/null()/*...*/))), &root_builder);

[wesm]

Old code

[wesm]

Thank you for the feedback. I'll merge this once the build passes

[wesm]

Travis CI: https://travis-ci.org/wesm/arrow/builds/566680039
Appveyor: https://ci.appveyor.com/project/wesm/arrow/builds/26412833

The Appveyor build isn't done yet so I'll wait a little bit to make sure MinGW is ok

[wesm]

One of the Windows builds is hanging, it doesn't appear related to this patch because I found it in a build in master. I opened

https://issues.apache.org/jira/browse/ARROW-6108

Merging this in the meantime