Ordered compaction and inlining with full catalog integration

[Alex-Monahan]

Hi folks!

This is a new PR meant to solve the same use case as #593, but addressing the PR feedback! Thank you for the guidance - it was super helpful. I am still open to any changes you recommend!

This adds 2 new tables to the DuckLake spec: ducklake_sort_info and ducklake_sort_expression.

ducklake_sort_info keeps a version history of the sort settings for tables over time. It has 1 row per time that a table has a new sort setting applied. (The prior PR used an option for this, but that has been removed based on the feedback).

ducklake_sort_expression tracks the details of that sort. For each time a new sort setting is applied, this table includes one row for each expression in the order by. (If I order by column3 asc, column42 desc, column64 asc, then there will be 3 rows.)

There are still a few limitations with this PR:

1. This does not order during insert (only during compaction and inline flush).
   • I would love to try to do a follow up PR to add this!
2. Only explicit column names can be used in the sorting, not expressions.
   • I tried to implement this but could not, so I could use some help with this piece!
   • There is a friendly error message (and tests) to document this limitation.
   • The spec has a column expression, so the intention was to make the spec itself forwards-compatible with expression-oriented sorting.
3. Files are still selected for compaction based on insertion order. It could be better to sort the list of files by min/max metadata before selecting files for compaction.
   • Let me know if this is desirable and I can work on it after the "order during insert" in number 1!

I believe that I made this fully compatible with the batching code, but I was testing locally on a DuckDB catalog and not on Postgres. Any extra eyes on that side would be great!

If this looks good, I can also do any docs PRs that you recommend - happy to help there.

Thanks folks! CC @philippmd as well as an FYI

[Alex-Monahan]

Welcome back @Mytherin , hope you had an excellent time away! I'd love your feedback - happy to address changes big or small. Sorry to ping, I just know that DuckLake 1.0 is not far away, and it takes me a bit of time to make C++ edits! :-)

Many thanks!!

[Mytherin]

Thanks! I've left a comment on the spec side of things - haven't looked at the code

[Alex-Monahan]

I've added in that separate table! Existing tests are passing with the join added - let me know if there are other things worth testing now that there are 2 tables involved!

Mind kicking off CI for me? I appreciate it!

[Alex-Monahan]

I realized that I was allowing the SET SORTED BY command to complete even when the columns were not valid (a legacy from the old option approach that was hard to validate up front). I've now restricted it so that it will fail if the column is incorrect and added some tests!

[pdet]

Hey Alex, the PR is looking super solid, thanks again for all the effort you put in!

I'm wondering if you could also add the following tests:

1. If you drop a table with an existing order, and then create a table with the same name but different columns?
2. If you get the changes information on snapshot()
3. If the alter statement won't intefeer with compaction (see, #671)
4. If we can also verify the new parquet files are correctly written directly (without acessing them from the table), and also if different files have the correct orders, when having different orders over multiple snapshots.
5. More transaction tests, for example, what happens if we begin a transaction and in the same transaction we add a new sorting order and alter a column name?
6. What happens if you do a batch insertion on a table with a set column order (not inlined)

The test files you are pretty detailed, but my bio llm context window is small, do you mind also moving them to a sorted_table folder and modularizing them a bit more?

I also guess in general we can do StringUtil::Lower() for catalog access when the result can be case insensitive.

[pdet]

I'm wondering if we should error if it is an unsupported dialect?

[Alex-Monahan]

Ah, I will look at the macros as an example. Would you prefer that I filter them out in SQL when I pull from the catalog?

[pdet]

This is more of a question, I'm open to suggestions. On one hand, we can be more permissive with anything dialect related, but I fear this might cause confusion, while an error removes any kind of ambiguity of the system's state.

[Alex-Monahan]

Looking back at this, the reason I implemented it this way was in case we wanted to deliberately have 2 sort orders active at once: one for DuckDB, and one for another engine like Spark for example. So if there were 2 separate sets of sorts, skipping the non-DuckDB ones (but not throwing an error) should allow them to co-exist.

Unfortunately, this case is very difficult to test in this repo! I think as catalog editing gets added on the Spark side, we can do some testing in that repo.

So, do you mind if we leave it as-is?

[Alex-Monahan]

Hey Alex, the PR is looking super solid, thanks again for all the effort you put in!

I'm wondering if you could also add the following tests:

1. If you drop a table with an existing order, and then create a table with the same name but different columns?
2. If you get the changes information on snapshot()
3. If the alter statement won't intefeer with compaction (see, Change schema tracking from global to table. #671)
4. If we can also verify the new parquet files are correctly written directly (without acessing them from the table), and also if different files have the correct orders, when having different orders over multiple snapshots.
5. More transaction tests, for example, what happens if we begin a transaction and in the same transaction we add a new sorting order and alter a column name?
6. What happens if you do a batch insertion on a table with a set column order (not inlined)

The test files you are pretty detailed, but my bio llm context window is small, do you mind also moving them to a sorted_table folder and modularizing them a bit more?

I also guess in general we can do StringUtil::Lower() for catalog access when the result can be case insensitive.

Thanks as always for the feedback! These tests make sense and I'm working on them! I had a few follow up questions.

On 3, would you like for me to validate that an alter table statement on a different table does not interfere? (Ahead of time or in the same transaction?) Based on #684, I was assuming that any alter table on the same table would be a hard line that prevents compaction - please let me know if I am wrong!

On 4, let me try to repeat back to you what I think you are looking for! I think you want to test the situation where I insert into the same table 3 times, each time with a different sort order setting, and then I run 1 compaction on that table. My goal was to only consider the most recent ducklake_sort_expression setting, regardless of the value of the setting at insert time, so this test would ensure that! I could also test multiple compactions and make sure that the final compaction resorts to the latest sort setting.

On 6, I'm not sure exactly how to proceed - I could use your help! This PR does not include the capability to do the sorting during an insert, so I'm not sure how the features would intersect just yet. Does adjusting the column order during insert result in the parquet file having the columns stored in a different order? I'm not sure what behavior I should be exercising with the test. Just let me know!

[pdet]

Hey Alex, the PR is looking super solid, thanks again for all the effort you put in!
I'm wondering if you could also add the following tests:

1. If you drop a table with an existing order, and then create a table with the same name but different columns?
2. If you get the changes information on snapshot()
3. If the alter statement won't intefeer with compaction (see, Change schema tracking from global to table. #671)
4. If we can also verify the new parquet files are correctly written directly (without acessing them from the table), and also if different files have the correct orders, when having different orders over multiple snapshots.
5. More transaction tests, for example, what happens if we begin a transaction and in the same transaction we add a new sorting order and alter a column name?
6. What happens if you do a batch insertion on a table with a set column order (not inlined)

The test files you are pretty detailed, but my bio llm context window is small, do you mind also moving them to a sorted_table folder and modularizing them a bit more?
I also guess in general we can do StringUtil::Lower() for catalog access when the result can be case insensitive.

Thanks as always for the feedback! These tests make sense and I'm working on them! I had a few follow up questions.

On 3, would you like for me to validate that an alter table statement on a different table does not interfere? (Ahead of time or in the same transaction?) Based on #684, I was assuming that any alter table on the same table would be a hard line that prevents compaction - please let me know if I am wrong!

Basically, we should check that the alter statement does not increase the schema counter of a table, since the alter statement here does not change the actual schema, and the sorting is somewhat "optional" I believe that files should still be compactable before/after the alter.

On 4, let me try to repeat back to you what I think you are looking for! I think you want to test the situation where I insert into the same table 3 times, each time with a different sort order setting, and then I run 1 compaction on that table. My goal was to only consider the most recent ducklake_sort_expression setting, regardless of the value of the setting at insert time, so this test would ensure that! I could also test multiple compactions and make sure that the final compaction resorts to the latest sort setting.

I wanted to see if files would always be generated with the latest sorting (e.g., by flushing) , and also compacted with the latest sorting, but now that I think about it again, maybe is an overkill because you already have similar tests to that, let's skip it.

On 6, I'm not sure exactly how to proceed - I could use your help! This PR does not include the capability to do the sorting during an insert, so I'm not sure how the features would intersect just yet. Does adjusting the column order during insert result in the parquet file having the columns stored in a different order? I'm not sure what behavior I should be exercising with the test. Just let me know!

Let's skip this.

[Alex-Monahan]

When investigating 3, I found that you are correct that I am currently incrementing the schema_version when altering the sort order. Dang! I think there are few other cases where we probably want to have the same "alter table, but don't change the schema_version". I'm thinking:

• SET_SORT_KEY
• SET_COMMENT
• SET_COLUMN_COMMENT

I'll start trying to implement that behavior, but let me know if I need to course correct! From my first prototype, it requires some additional complexity in a few spots (I think we will need a different construct than new_tables for storing those kinds of alters), so I'm open to feedback and ideas!

[pdet]

When investigating 3, I found that you are correct that I am currently incrementing the schema_version when altering the sort order. Dang! I think there are few other cases where we probably want to have the same "alter table, but don't change the schema_version". I'm thinking:

• SET_SORT_KEY
• SET_COMMENT
• SET_COLUMN_COMMENT

I'll start trying to implement that behavior, but let me know if I need to course correct! From my first prototype, it requires some additional complexity in a few spots (I think we will need a different construct than new_tables for storing those kinds of alters), so I'm open to feedback and ideas!

Yes, I think none of those should impact the schema ranges for compaction. I think the main thing you have to do is to ensure that these statements won't write a new entry into ducklake_schema_versions, as this table keeps track of schema changes per ducklake table.

[Alex-Monahan]

Yup, I will prevent that! Unfortunately, I'm also finding that I need to update the local schema cache (since usually a new schema version would be handled differently). I'm making progress!