JFinis commented on code in PR #196:
URL: https://github.com/apache/parquet-format/pull/196#discussion_r1237381221
##########
src/main/thrift/parquet.thrift:
##########
@@ -966,6 +985,23 @@ struct ColumnIndex {
/** A list containing the number of null values for each page **/
5: optional list<i64> null_counts
+
+ /**
+ * A list of Boolean values to determine pages that contain only NaNs. Only
+ * present for columns of type FLOAT and DOUBLE. If true, all non-null
+ * values in a page are NaN. Writers are suggested to set the corresponding
+ * entries in min_values and max_values to NaN, so that all lists have the
same
+ * length and contain valid values. If false, then either all values in the
+ * page are null or there is at least one non-null non-NaN value in the page.
+ * As readers are supposed to ignore all NaN values in bounds, legacy readers
+ * who do not consider nan_pages yet are still able to use the column index
+ * but are not able to skip only-NaN pages.
+ */
+ 6: optional list<bool> nan_pages
Review Comment:
Yes, number of rows in the offset index isn't enough due to repeated values.
Apart from this, the suggestions seem to turn a bit in circles now. Note
that all suggestions in this thread were already mentioned in [my earlier post
where I depicted our possible options for the column
index](https://github.com/apache/parquet-format/pull/196#issuecomment-1486416762).
@pitrou what you mentioned was my Option 2. I personally would prefer this
as it feels like a useful thing to have anyway. Having said that, others
pointed rightfully out that it would cost a few bytes even for non float
columns. The value might be valuable for other tasks as well. For example, it
could be used to quickly check how many nested values are in a page. By having
these values one could sum up the nested values per column chunk by adding up
all the value counts. This is currently a value that cannot be optained at all
through statistics; instead one has to decode pages and count. For example, the
SQL query `SELECT count(*) FROM some_nested_column;` could be fully answered
with such a value_counts field.
@wgtmac your proposal was my Option 1 and actually my initial proposal (see
previous commit). Note that you
[earlier](https://github.com/apache/parquet-format/pull/196#pullrequestreview-1362171450)
actually were against writing NaNs and rather preferred the nan_pages approach:
> Personally speaking, apart from adding a nan_count to the statistics, I
would go with the option 3: adding a nan_pages bool list to the column index. I
am not in favor of writing any NaN to min/max bounds.
Is your argument that if we now need to write the NaNs anyway, that we
should in this case just use them instead of adding nan_pages? I do agree that
this would save the extra field and I personally see nothing wrong in doing
this. Readers need to be able to detect NaN values anyway (to ignore them), so
readers should be able to use the same logic to determin min=max=NaN <=> all
values are NaN.
As mentioned in my previous post where I compared the three approaches, I am
happy to implement any of them and I think all of them will fulfill the
requirements. In my personal opinion, I like the current approach with
nan_pages actually the least, as it seems redundant if we have to write NaN
values anyway and I see no problem in using NaN values for the "all values NaN
check".
I also like the option of adding a value_counts field to the column index of
all columns. It feels like a useful and missing field (that is not subsumed by
offset index row counts for nested columns) and I would love to add it as well
and I feel the few extra bytes will be so negligible in contrast to the actual
data that no-one will ever care. Also it would enable us to do the check for
all values NaN the same way in page statistics and in the column index.
So we're back at the three options I proposed:
1. Drop nan_pages and use my initial approach of "min=max=NaN <=> all values
are NaN" in the column index
2. Drop nan_pages and instead add value_counts so we can use
value_counts-null_counts==nan_counts to determine whether all values are null.
(My personal favorite)
3. Retain the current state and use `nan_pages`
@wgtmac @mapleFU @gszadovszky @pitrou could we arrive at a consensus here?
I'm happy to adapt my PR to any of the solutions. @gszadovszky you also haven't
mentioned your favorite, yet (you just pointed out that we have to write some
valid value).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
