feat: writing/reading to parquet - #1002
Conversation
tmadlener
left a comment
There was a problem hiding this comment.
I haven't looked into any of the details so far. I have a conceptual question; Currently this would create directories with files. Would it make sense to grab libarchive and "tar those up" on the fly so that in the end we deal with one "file" again?
I am not sure why this would help. From a user perspective, a directory with multiple files would be easier to use directly in python environments. |
tmadlener
left a comment
There was a problem hiding this comment.
I see your point, fair enough. It would in any case be fairly straight forward to fix that later.
I have had a first look and in general there seem to be mainly minor things. Just to confirm: The tests are essentially copies from the other I/O backends just using the Arrow{Writer,Reader} instead, right?
Final question, is Arrow{Reader,Writer} the best name, or should it also contain Parquet somehow?
Yes tests are copies from other backends. Have no strong opinion on the name though cause my initial thought was ParquetWriter/Reader itself but that didn't have Arrow so I wasn't a fan. |
|
Have addressed all the comments. |
|
I don't know if this is elsewhere but there are a few things that would be good or necessary to have:
|
I think podio-dump already works with parquet files will check this regardless. |
Podio dump does work. |
veprbl
left a comment
There was a problem hiding this comment.
Sorry, meant to post this earlier.
| }); | ||
|
|
||
| registry.registerReader(typeName, | ||
| [](const std::shared_ptr<arrow::Array>& array, int64_t rowIndex, bool isSubset, |
There was a problem hiding this comment.
This is clang formattted. The rules come up as errors in pre-commit so nothing much I can do about it.
There was a problem hiding this comment.
I suspect if you add a newline after ( it won't undo it.
There was a problem hiding this comment.
You could try that, but otherwise clang-format decides here, I would say.
There was a problem hiding this comment.
I tried and clang format just un-did my changes so I don't think this would be possible.
| #else | ||
| throw std::runtime_error("SIO writer not available. Please recompile with SIO support."); | ||
| #endif | ||
| } else if (endsWith(filename, ".arrow") || lower(type) == "arrow") { |
There was a problem hiding this comment.
| } else if (endsWith(filename, ".arrow") || lower(type) == "arrow") { | |
| } else if (endsWith(filename, ".podio_parquet") || lower(type) == "parquet") { |
Does it make sense to be explicit about the flavor of arrow format?
Also I would not simply say "arrow" without denoting that this is a podio layout.
There was a problem hiding this comment.
#1002 (comment) I just changed this to arrow. Also btw, this is the directory name in Arrow's case so arrow should suffice. The files inside are category_name.parquet and metadata.json.
There was a problem hiding this comment.
Yes, this is the suggestion to the current state.
There was a problem hiding this comment.
Have changed this.
|
@tmadlener @jmcarcell @veprbl Can I get a list of things to be resolved/added so that this could be moved to closure? I am a bit confused with so many comments still open. |
|
I have resolved a few of the conversations that you addressed. From my point of view there is essentially nothing left to do here apart from some minor things where @veprbl and @jmcarcell have originally commented. (I have added a suggestion from my side to those, but will let them have the final say). |
|
Have addressed everything now. |
|
Looks like everyone is happy now. I will merge this later today, unless there are last minute comments. I have created an issue to keep track of the potential optimisation (#1009). |
4b03b36 to
0a3eef6
Compare
Closes #1006
This PR extends the existing Arrow backend in Podio by adding the capability to write and read datamodels directly to and from Apache Parquet files. This significantly improves data interoperability with Python ecosystems and column-oriented analysis workflows.
Key Features & Additions:
BEGINRELEASENOTES
ENDRELEASENOTES