Fix OrderPreservationType issue of MATERIALIZED CTEs #10587

kryonix · 2024-02-12T09:56:47Z

Materialized CTEs did sometimes create the incorrect OrderPreservationType. This is because OrderPreservationRecursive tried to take the materialization phase into account, which should not be done.

Fixes #10576, reverts and fixes #10560.

The tests will likely fail, however, because this revealed another—unrelated—bug:

test/sql/function/list/list_reverse.test

WITH example AS (
  SELECT [1, 2, 3] AS arr UNION ALL
  SELECT [4, 5] AS arr UNION ALL
  SELECT [] AS arr
)
SELECT
  list_reverse(arr) AS reverse_arr
FROM example ORDER BY length(reverse_arr) DESC;

Invalid Error: Unoptimized statement differs from original result!
Original Result:
reverse_arr
INTEGER[]
[ Rows: 3]
[3, 2, 1]
[5, 4]
[]

Unoptimized:
reverse_arr
INTEGER[]
[ Rows: 3]
[1, 2, 3]
[4, 5]
[]

kryonix · 2024-02-12T15:07:17Z

I did investigate the failing test case a bit further (simplified query):

SELECT arr, list_concat(list_reverse(arr), arr) FROM (SELECT [1,2,3]) AS _(arr);
┌───────────┬─────────────────────────────────────┐
│ arr       ┆ list_concat(list_reverse(arr), arr) │
╞═══════════╪═════════════════════════════════════╡
│ [3, 2, 1] ┆ [1, 2, 3, 1, 2, 3]                  │
└───────────┴─────────────────────────────────────┘
   ↑  ↑  ↑     ↑  ↑  ↑ 
   Bug 1       Bug 2

This query contains two bugs. First, column arr should be [1,2,3], but it is reversed. This is a bug in array_slice.cpp, line 349: ListVector::ReferenceEntry(result, list_or_str_vector);. The input is just referenced, but I think the vector has to be copied. Otherwise, the input is changed.

eeb0093 fixes the second bug, where list_reverse has no effect when nested in list_concat.

Again, this is totally unrelated to the initial issue I am trying to address with this pr.

kryonix · 2024-02-12T20:46:42Z

Alright. I think I fixed it. However, I'm not sure if this fix of the array slicing code has a negative effect on performance due to copy overhead. It now copies the input vector just as list_sort does.

Mytherin

Thanks for the fixes! LGTM. Perhaps @taniabogatsch can have a look at the changes to the list functions?

taniabogatsch

Hi @kryonix, thanks for the PR and for uncovering these bugs!

I think we need to look at them more in-depth before merging this. Especially for the second bug (list_reverse inside list_concat). I think the bug is not in the list_concat code, i.e., that code just receives incorrect input data.

Is it possible to merge these fixes without the changes to list_concat/list_slice? If so, I'd suggest that we open a separate issue/PR to address them separately?

taniabogatsch · 2024-02-13T10:42:25Z

src/core_functions/scalar/list/array_slice.cpp

-	Vector &list_or_str_vector = args.data[0];
+	Vector &list_or_str_vector = result;
+	// this ensures that we do not change the input chunk
+	VectorOperations::Copy(args.data[0], list_or_str_vector, count, 0, 0);


However, I'm not sure if this fix of the array slicing code has a negative effect on performance

Indeed, fixing this bug by copying the input vector is not the most efficient way to address this. However, I think we can merge this, and address the performance in a separate PR. cc @maiadegraaf

taniabogatsch · 2024-02-13T10:57:26Z

src/function/scalar/list/list_concat.cpp

@@ -55,14 +55,12 @@ static void ListConcatFunction(DataChunk &args, ExpressionState &state, Vector &
 		if (lhs_data.validity.RowIsValid(lhs_list_index)) {
 			const auto &lhs_entry = lhs_entries[lhs_list_index];
 			result_entries[i].length += lhs_entry.length;
-			ListVector::Append(result, lhs_child, *lhs_child_data.sel, lhs_entry.offset + lhs_entry.length,
-			                   lhs_entry.offset);
+			ListVector::Append(result, lhs_child, lhs_entry.offset + lhs_entry.length, lhs_entry.offset);


We cannot assume the child vector to be a flat vector, i.e., removing the *lhs_child_data.sel. I may be missing something. But I think this might be related (to an extent) to the other bug in list_slice, as list_reverse uses list_slice under the hood. I.e., I think we need to fix this somewhere else, so that we retrieve a correct selection vector for our child vector here.

Feel free to revert this. I'm not too familiar with this part of the system. For my test cases (and the unit tests) it worked fine. But I may not see the whole picture.

We might be missing a unit test here that would cause this to fail if the child vector is not a flat vector. Let's double-check this and then either address the fix properly or we can confirm that this fix works! I think @maiadegraaf already confirmed that she'll have a closer look. :)

Awesome! I reverted the change.

kryonix · 2024-02-13T11:07:32Z

Is it possible to merge these fixes without the changes to list_concat/list_slice? If so, I'd suggest that we open a separate issue/PR to address them separately?

Yes it is. I only had a look at these issues because I introduced a new test case that does not work because of these bugs.

The important part of this pr is 9543363

taniabogatsch · 2024-02-13T11:27:36Z

Yes it is. I only had a look at these issues because I introduced a new test case that does not work because of these bugs.

Great - would it be okay for you to revert the changes here, and then we can merge this?

Mytherin · 2024-02-19T11:39:03Z

Thanks!

Merge pull request duckdb/duckdb#10658 from hannes/csvpathlength Merge pull request duckdb/duckdb#10756 from Mytherin/preserveinsertionordermemory Merge pull request duckdb/duckdb#10746 from samansmink/enable-azure-autoload Merge pull request duckdb/duckdb#10747 from maiadegraaf/list_reverse_bug Merge pull request duckdb/duckdb#10748 from taniabogatsch/capi-tests Merge pull request duckdb/duckdb#10739 from peterboncz/pb/immmedate_mode_only_in_non_autocommit Merge pull request duckdb/duckdb#10688 from Tmonster/union_exclude Merge pull request duckdb/duckdb#10710 from samansmink/comment-on-column Merge pull request duckdb/duckdb#10725 from hawkfish/fuzzer-preceding-frame Merge pull request duckdb/duckdb#10723 from hawkfish/fuzzer-null-timestamp Merge pull request duckdb/duckdb#10436 from taniabogatsch/map-fixes Merge pull request duckdb/duckdb#10587 from kryonix/main Merge pull request duckdb/duckdb#10738 from TinyTinni/fix-assert-in-iscntrl Merge pull request duckdb/duckdb#10708 from carlopi/ci_fixes Merge pull request duckdb/duckdb#10726 from hawkfish/fuzzer-to-weeks Merge pull request duckdb/duckdb#10727 from hawkfish/fuzzer-window-bind Merge pull request duckdb/duckdb#10733 from TinyTinni/remove-static-string Merge pull request duckdb/duckdb#10715 from Tishj/python_tpch_regression_rework Merge pull request duckdb/duckdb#10728 from hawkfish/fuzzer-argminmax-decimal Merge pull request duckdb/duckdb#10717 from carlopi/fix_extension_deploy Merge pull request duckdb/duckdb#10694 from Mytherin/castquerylocation Merge pull request duckdb/duckdb#10448 from peteraisher/feature/use-assertThrows-for-jdbc-tests Merge pull request duckdb/duckdb#10691 from Mytherin/issue10685 Merge pull request duckdb/duckdb#10684 from Mytherin/distincton Merge pull request duckdb/duckdb#9539 from Tishj/timestamp_unit_to_tz Merge pull request duckdb/duckdb#10341 from Tmonster/tpch_ingestion_benchmark Merge pull request duckdb/duckdb#10689 from Mytherin/juliaversion Merge pull request duckdb/duckdb#10669 from Mytherin/skippedtests Merge pull request duckdb/duckdb#10679 from Tishj/reenable_window_rows_overflow Merge pull request duckdb/duckdb#10672 from carlopi/wasm_extensions_ci Merge pull request duckdb/duckdb#10660 from szarnyasg/update-storage-info-for-v0100 Merge pull request duckdb/duckdb#10643 from bleskes/duck_transaction_o11y Merge pull request duckdb/duckdb#10654 from carlopi/fix_10548 Merge pull request duckdb/duckdb#10650 from hannes/noprintf Merge pull request duckdb/duckdb#10649 from Mytherin/explicitenumnumbers

Fix OrderPreservationType issue of MATERIALIZED CTEs

9543363

szarnyasg requested a review from Mytherin February 12, 2024 09:59

Fix list_concat selection vector issue

eeb0093

github-actions bot marked this pull request as draft February 12, 2024 15:11

Mytherin marked this pull request as ready for review February 12, 2024 19:37

Mytherin marked this pull request as draft February 12, 2024 19:40

Fix array slicing bug

0ff25a6

kryonix marked this pull request as ready for review February 12, 2024 20:42

Mytherin requested a review from taniabogatsch February 13, 2024 08:23

Mytherin approved these changes Feb 13, 2024

View reviewed changes

taniabogatsch suggested changes Feb 13, 2024

View reviewed changes

Revert eeb0093

dee8d69

github-actions bot marked this pull request as draft February 13, 2024 11:39

kryonix marked this pull request as ready for review February 13, 2024 12:00

kryonix requested a review from taniabogatsch February 14, 2024 13:29

taniabogatsch approved these changes Feb 14, 2024

View reviewed changes

maiadegraaf added the Ready To Merge label Feb 15, 2024

Mytherin merged commit 6f1d82f into duckdb:main Feb 19, 2024

maiadegraaf mentioned this pull request Feb 19, 2024

List_slice bug fix #10747

Merged

kryonix mentioned this pull request Apr 3, 2024

Issue found on page 'Nested Functions': ARRAY_REVERSE (not working) vs. ARRAY_REVERSE_SORT (working) #11491

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix OrderPreservationType issue of MATERIALIZED CTEs #10587

Fix OrderPreservationType issue of MATERIALIZED CTEs #10587

kryonix commented Feb 12, 2024 •

edited

Loading

Uh oh!

kryonix commented Feb 12, 2024 •

edited

Loading

Uh oh!

kryonix commented Feb 12, 2024 •

edited

Loading

Uh oh!

Mytherin left a comment

Uh oh!

taniabogatsch left a comment

Uh oh!

taniabogatsch Feb 13, 2024

Uh oh!

taniabogatsch Feb 13, 2024

Uh oh!

kryonix Feb 13, 2024

Uh oh!

taniabogatsch Feb 13, 2024

Uh oh!

kryonix Feb 13, 2024

Uh oh!

kryonix commented Feb 13, 2024 •

edited

Loading

Uh oh!

taniabogatsch commented Feb 13, 2024

Uh oh!

Mytherin commented Feb 19, 2024

Uh oh!

Uh oh!

Fix OrderPreservationType issue of MATERIALIZED CTEs #10587

Fix OrderPreservationType issue of MATERIALIZED CTEs #10587

Conversation

kryonix commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kryonix commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kryonix commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mytherin left a comment

Choose a reason for hiding this comment

Uh oh!

taniabogatsch left a comment

Choose a reason for hiding this comment

Uh oh!

taniabogatsch Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

taniabogatsch Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

kryonix Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

taniabogatsch Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

kryonix Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

kryonix commented Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taniabogatsch commented Feb 13, 2024

Uh oh!

Mytherin commented Feb 19, 2024

Uh oh!

Uh oh!

kryonix commented Feb 12, 2024 •

edited

Loading

kryonix commented Feb 12, 2024 •

edited

Loading

kryonix commented Feb 12, 2024 •

edited

Loading

kryonix commented Feb 13, 2024 •

edited

Loading