Clarify num-nulls handling in Statistics and ColumnIndex

[mapleFU]

Rationale for this change

Clarify num-nulls handling. I've mentioned this in maillist: https://lists.apache.org/thread/oqd9lt7p0jtf217hg0667xhk0zvwbgvt

What changes are included in this PR?

Suggest writer to have num_nulls/null_count

Do these changes have PoC implementations?

no

[wgtmac]

Suggested change

	* Writers should write this field even if it is zero or in non-null columns.
	* Writers SHOULD always write this field even if it is zero (a.k.a. no null value)

[wgtmac]

We might also add the expectation to the reader implementations?

[mapleFU]

We might also add the expectation to the reader implementations?

I'd like to wait other replying the context in ML firstly

[mapleFU]

@pitrou @gszadovszky @emkornfield @alamb @etseidl @shangxinli Would you mind take a look?

[etseidl]

Thanks @mapleFU, I think this is an important clarification. Will you also add this language to ColumnIndex?

[mapleFU]

Will you also add this language to ColumnIndex?

Sure, wait 1h

[etseidl]

Will you also add this language to ColumnIndex?

Sure, wait 1h

I'll be asleep 🤣

[mapleFU]

Aha, I may need sometime to check the parquet-java, parquet-rs ColumnIndex impl, so maybe would take some time, it would be finished before you wake up tomorrow. Nice dream!

[gszadovszky]

@mapleFU, it looks good to me and is a proper change for the actual case.

However, the statement If null_count is not present, readers SHOULD NOT assume null_count == 0. should be a general one for each optional values in this file. We've had a couple of issues not checking whether a field is actually set and using its default value.
Maybe SHOULD would be better being a MUST.

[mapleFU]

For column index:

• Parquet-C++: https://github.com/apache/arrow/blob/4af1e491df7ac22217656668b65c3e8d55f5b5ab/cpp/src/parquet/page_index.cc#L459
• Parquet-rs: https://github.com/apache/arrow-rs/blob/6c59b7637592e4b67b18762b8313f91086c0d5d8/parquet/src/file/metadata/mod.rs#L1273
• Parquet-java: https://github.com/apache/parquet-java/blob/d4384d3f2e7703fab6363fd9cd80a001e9561db2/parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/ColumnIndexBuilder.java#L723

In parquet-rs seems it always write? And in C++/Java If page statistics has null counts, it's bring up to ColumnIndex.

[pitrou]

LGTM, just two wording suggestions

[alamb]

Thank you @mapleFU -- this makes sense to me

[alamb]

I agree with this statement (even though there is an issue with older parquet files written with parquet-rs)

[JFinis]

What did parquet-rs do? Not write the null count in case it was null? If so, then readers will just not optimize out null handling in this case, which seems fine.

[mapleFU]

@JFinis apache/arrow-rs#6257 would support write 0 here
Before this nonthing is writen.

[etseidl]

One last nit 😄. Thanks again!

[mapleFU]

@gszadovszky Would you mind take a look?

Besides, I didn't get any response in the maillist. What should I do before checking in this pr?

[gszadovszky]

@mapleFU, I think this PR does not change the way an implementation should handle these values. It only articulates the correct handling.
Also, the PR has been approved by a quite broad audience already. You may safely push it.

[mapleFU]

Merge now, thanks all!

[JFinis]

Why should writers write this field if the column isn't nullable? That feels strictly redundant and therefore a waste of memory. If a reader assumes that their could be nulls even though the type is required in the schema, then this reader has a bug. There wouldn't even be a way to write nulls into a required column, as they cannot be represented in this case.

(Also, if you disregard this comment, at least change the wording from not nullable to not required, as that's the correct term w.rt. to the schema (there is no nullable field in the schema, just a repetition type).)

[mapleFU]

Why should writers write this field if the column isn't nullable?

I agree it's not costless in this section, however most impl uses this. It's not a standard but it's widely used.

(Also, if you disregard this comment, at least change the wording from not nullable to not required, as that's the correct term w.rt. to the schema (there is no nullable field in the schema, just a repetition type).)

You're right, I'll rechecking this

[JFinis]

null_count => null_counts

[mapleFU]

@JFinis I totally agree the non-nullable ( including def-level in parquet) type can implicit deducing that not-null, however I found most implemention consumes that and produce that, so I sound it here.