Dictionary page offset logic

[sclmn]

I was looking at this code and wondered about the checks.

• I'm not sure why there is a check that the dictionary page offset is greater than 0? If this isn't a dictionary page, should it be not set (first condition)?
• Is it possible for the data page offset to equal 0 (when we don't have data pages)?

[emkornfield]

I'm not sure why there is a check that the dictionary page offset is greater than 0? If this isn't a dictionary page, should it be not set (first condition)?

Looking at the code flow, if the file is malformed (with a negative dictionary_page_offset) we potentially fail to catch the negative offset on line 189

Is it possible for the data page offset to equal 0 (when we don't have data pages)?

Technically, for a valid parquet file neither offset should be zero because parquet has a magic number as its first four bytes. Without data pages it means the row group is empty, and in theory readers should skip it (e.g. this PR does this)

[emkornfield]

It does look there were some strange behavior for offset = 0 (see the change that added this: 7a9ba61)

[sclmn]

Looking at the code flow, if the file is malformed (with a negative dictionary_page_offset) we potentially fail to catch the negative offset on line 189

I think the current code sets the col_start to the data_page_offset since we have the condition column_metadata->dictionary_page_offset() > 0 ... so I'm not sure how line 189 returns an error for a negative dictionary offset.

Technically, for a valid parquet file neither offset should be zero because parquet has a magic number as its first four bytes. Without data pages it means the row group is empty, and in theory readers should skip it (e.g. this apache/parquet-java#1018 does this)

The current code has the condition

col_start > column_metadata->dictionary_page_offset())

If the data_page_offset is 0 (no data page), it will prevent us from setting col_start to the dictionary page offset (for valid positive dictionary offset). This will trigger an error.

[emkornfield]

I think the current code sets the col_start to the data_page_offset since we have the condition column_metadata->dictionary_page_offset() > 0 ... so I'm not sure how line 189 returns an error for a negative dictionary offset.

You are right, I read it too quickly, I think the answer is java wrote out zero values.

If the data_page_offset is 0 (no data page), it will prevent us from setting col_start to the dictionary page offset (for valid positive dictionary offset). This will trigger an error.

I'm not sure what writers end up populating this field with if the values are no data pages, but ideally we wouldn't get here because we already no there are zero rows from the row group metadata?

[sclmn]

I'm not sure what writers end up populating this field with if the values are no data pages, but ideally we wouldn't get here because we already no there are zero rows from the row group metadata?

I think the rust approach makes more sense. There are a few test cases where the data page offset is set to 0 in that repo.

[pitrou]

It does look there were some strange behavior for offset = 0 (see the change that added this: 7a9ba61)

The original issue being https://issues.apache.org/jira/browse/ARROW-5322 (not that it gives many details about how those files were generated)

[pitrou]

There are a few test cases where the data page offset is set to 0 in that repo.

A data page offset set to 0 means that there are no data pages (and therefore no data at all) in the row group?