Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C++] Order of row index streams does not match the order of streams in the file footer #1475

Open
vuule opened this issue Apr 21, 2023 · 10 comments

Comments

@vuule
Copy link

vuule commented Apr 21, 2023

When writing a file with a string column and multiple row groups, the resulting file has incorrect row index streams.
The string column is encoded using direct encoding. The file footer contains the LENGTH (kind 2) stream before DATA (kind 1) stream. However, the row index seems to contain the index data for the DATA stream before the LENGTH stream. Switching out the order in which we read the row index streams fixes the issue and everything can be used correctly.

Isolation:
Only observing this behavior with string columns. Other types with multiple streams look correct in this regard.
Behavior looks unrelated to string content in the column.
No info on dictionary encoded string columns - writer seemlingly defaults to direct encoding.

See attached repro file. The file contains a single string column, with ["*"] * 10001
10001_strings.zip

@vuule
Copy link
Author

vuule commented Apr 21, 2023

Logs that point to incorrect order: rapidsai/cudf#11890 (comment)

@wgtmac
Copy link
Member

wgtmac commented Apr 23, 2023

Thanks for reporting the issue! @vuule

The order of data streams are NOT FIXED meaning that:

  • In a direct-encoded string columns, DATA stream can be placed BEFORE or AFTER LENGTH stream. Same flexibility for PRESENT stream.
  • Even data streams of different columns can be interleaved.

However, the order of positions in a index stream is FIXED. So for a direct-encoded string column, its INDEX stream always put positions in this order: PRESENT stream (if exists), DATA stream and LENGTH stream.

I checked the specs and it does not state this clearly. It would be a good time to document this as well. @deshanxiao

@deshanxiao
Copy link
Contributor

Thank you @wgtmac , I will add these to #1465.

@wgtmac
Copy link
Member

wgtmac commented Apr 24, 2023

Thank you @wgtmac , I will add these to #1465.

Thanks @deshanxiao ! Could you also verify that the java implementation matches this behavior?

@vuule
Copy link
Author

vuule commented Apr 24, 2023

Thank you for the clarification @wgtmac !
What about other cases when a column has multiple streams? Is the order of index streams always the same as in the tables at https://orc.apache.org/specification/ORCv1/?

@wgtmac
Copy link
Member

wgtmac commented Apr 25, 2023

Yes, the order is fixed. This is implemented in the recordPosition call as below.

In the TreeWriterBase.java, positions of present stream are recorded first.

/**
* Record the current position in each of this column's streams.
* @param recorder where should the locations be recorded
*/
void recordPosition(PositionRecorder recorder) throws IOException {
if (isPresent != null) {
isPresent.getPosition(recorder);
}
}

And then in the StringBaseTreeWriter.java, positions of data stream and length stream are recorded in order.

private void recordDirectStreamPosition() throws IOException {
if (rowIndexPosition != null) {
directStreamOutput.getPosition(rowIndexPosition);
lengthOutput.getPosition(rowIndexPosition);
}
}

I followed the same order when I was implementing the C++ writer so they should be consistent.

@wgtmac
Copy link
Member

wgtmac commented Apr 25, 2023

Thank you for the clarification @wgtmac ! What about other cases when a column has multiple streams? Is the order of index streams always the same as in the tables at https://orc.apache.org/specification/ORCv1/?

IIRC, the order is same as the table of the spec doc.

@deshanxiao
Copy link
Contributor

deshanxiao commented Apr 25, 2023

Yes, the order is fixed. This is implemented in the recordPosition call as below.

In the TreeWriterBase.java, positions of present stream are recorded first.

/**
* Record the current position in each of this column's streams.
* @param recorder where should the locations be recorded
*/
void recordPosition(PositionRecorder recorder) throws IOException {
if (isPresent != null) {
isPresent.getPosition(recorder);
}
}

And then in the StringBaseTreeWriter.java, positions of data stream and length stream are recorded in order.

private void recordDirectStreamPosition() throws IOException {
if (rowIndexPosition != null) {
directStreamOutput.getPosition(rowIndexPosition);
lengthOutput.getPosition(rowIndexPosition);
}
}

I followed the same order when I was implementing the C++ writer so they should be consistent.

Thank you for sharing the Java code. I double check it and you are right @wgtmac .

In a direct-encoded string columns, DATA stream can be placed BEFORE or AFTER LENGTH stream. Same flexibility for PRESENT stream.

In fact, different languages currently have different order implementations. The order of java depends on the method of compareTo to flush the stream to disk.

Even data streams of different columns can be interleaved.

Do you mean that the streams will cross for different columns like:
col1 streamtype1
col2 streamtype1
col1 streamtype2

I notice that the streams in the same column will appear together, but the order of the streams in different column is uncertain even they are the same data type.

@deshanxiao
Copy link
Contributor

BTW, Is it necessary for us to add a type list in IndexEntry to describe the type of the position? @wgtmac @dongjoon-hyun @guiyanakuang

@wgtmac
Copy link
Member

wgtmac commented Apr 25, 2023

Yes, that would help a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants