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

Support (order by / sort) for DataFrameWriteOptions #13874

Merged
merged 4 commits into from
Dec 24, 2024

Conversation

zhuqi-lucas
Copy link
Contributor

@zhuqi-lucas zhuqi-lucas commented Dec 21, 2024

Which issue does this PR close?

Closes #13873

Rationale for this change

DataFrameWriteOptions is missing an order by / sort by like available in SQL.

For sql we have the option to sort, for example:

You can use the WITH ORDER clause of the CREATE EXTERNAL TABLE if your data is already ordered

https://datafusion.apache.org/user-guide/sql/ddl.html#create-external-table

CREATE EXTERNAL TABLE test (
    c1  VARCHAR NOT NULL,
    c2  INT NOT NULL,
    c3  SMALLINT NOT NULL,
    c4  SMALLINT NOT NULL,
    c5  INT NOT NULL,
    c6  BIGINT NOT NULL,
    c7  SMALLINT NOT NULL,
    c8  INT NOT NULL,
    c9  BIGINT NOT NULL,
    c10 VARCHAR NOT NULL,
    c11 FLOAT NOT NULL,
    c12 DOUBLE NOT NULL,
    c13 VARCHAR NOT NULL
)
STORED AS CSV
-- this line tells DataFusion the data in the file is already ordered by (c2 ASC)
WITH ORDER (c2 ASC)
LOCATION '/path/to/aggregate_test_100.csv'
OPTIONS ('has_header' 'true');

But for writing my parquet or other format files, we don't support it

What changes are included in this PR?

Add the sort support.

Are these changes tested?

yes

Added unit testing.

Are there any user-facing changes?

Yes, we support new order option for DataFrameWriteOptions

@github-actions github-actions bot added the core Core DataFusion crate label Dec 21, 2024
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @zhuqi-lucas -- this PR is really nice in my mind -- it is well commented and well tested 🏅

cc @TheBuilderJR


assert_batches_eq!(
&[
"+---+---+",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a good test 👍

.sort(options.sort_by)?
.build()?
};

Copy link
Contributor

@Dandandan Dandandan Dec 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't LogicalPlanBuilder::copy_to have the ordering info as well?
If we write the info to a new table, do we know avoid sorting the table again? We should expose the ordering info to the newly created table for that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think copy_to inserts the data from whatever the input plan is (if there is a redundant sort the optimizer will remove it). Is that what you are asking?

It is an interesting question of how to communicate "this table is sorted" information for newly written files. Is this what you are talking about?

Copy link
Contributor

@Dandandan Dandandan Dec 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I mean that after this PR we seem we don't communicate "we are sorted" yet, which is I think would support usecases such as in the issue from @TheBuilderJR (write data sorted on a column, making following read query with order by on the same column fast).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @alamb @Dandandan for review, i am also interested how we can communicate "this table is sorted" information for newly written files. I will also investigate this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the ordering information is normally handled by a higher level "catalog" rather than the parquet format itself.

This is the only thing I know of in Parquet, but I don't think it can describe the ordering

https://github.com/apache/parquet-format/blob/d784f11f4485e64fdeaa614e0bde125f5132093d/src/main/thrift/parquet.thrift#L587-L591

Maybe this is something that https://iceberg.apache.org/ could represent 🤔

It is also conceivable that DataFusion itself could write custom metadata in paquet and other formats that support that custom metadata with the ordering, but that seems like we would just be reinventing Iceberg and similar table formats

@github-actions github-actions bot added documentation Improvements or additions to documentation sql SQL Planner logical-expr Logical plan and expressions optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) substrait common Related to common crate functions labels Dec 23, 2024
@github-actions github-actions bot removed documentation Improvements or additions to documentation sql SQL Planner logical-expr Logical plan and expressions optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) substrait common Related to common crate functions labels Dec 23, 2024
@zhuqi-lucas
Copy link
Contributor Author

Updated the code to also include the write_table testing case which is the same with insert into sql.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion this PR is a step forward as at least now it is easier to configure the writer desires a particular sorted output

However, as @Dandandan points out, while this might make it easier to find ordering during writing DataFusion can already do this via the DataFrame API (which is actually what this PR does under the covers) but there is no way to communicate the ordering of a file back with the existing listing table implementation

I recommend we file a follow on issue to discuss that usecase if people want to pursue it

@zhuqi-lucas
Copy link
Contributor Author

Thank you @alamb @Dandandan for review, it makes sense we continue investigating the solution for communicating the ordering of a file back with the existing listing table implementation.

I created a follow-up issue for this:
#13891

@Dandandan Dandandan merged commit 6cfd1cf into apache:main Dec 24, 2024
24 checks passed
@Dandandan
Copy link
Contributor

Thank you @zhuqi-lucas

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

Successfully merging this pull request may close these issues.

Support (order by / sort) for DataFrameWriteOptions
3 participants