Internal #215: Window EXCLUDE Functionality #9220

hawkfish · 2023-10-04T15:59:43Z

Implement the SQL windowing EXCLUDE functionality started by @j-w-moebius. This includes:

Parser and Serialisation extensions
Implementations and tests for all accelerators
Changes to the custom window API

Despite the increased complexity of the calling sequences, performance appears unaffected.

fixes: duckdblabs/duckdb-internal#215

Reverse the expected order of one test now that we have deterministic ordering of single partitions.

Move the code for WindowSegmentTree::EvaluateInternal to WindowSegmentTreeState. This hides the implementation details and prepares to fix some memory leaks.

Fix the right side memory leak. Avoid unnecessary per-chunk memory allocation.

Plumb the setting through and throw if it is not supported.

Change API to pass frame lists instead of window setting.

Support the EXCLUDE modifiers for COUNT(*).

Use swap instead of copying.

Convert ReuseIndexes to handle multiple ranges. Only tested for single range.

Reduce the insert range of Reuse to the current frames instead of the union of current and previous frames.

Back out range restriction. Add median tests.

Make the new exclude clause member optional so we don't have to bump the file version.

Still broken but should be closer. Probably just needs a `GetDefault`?

Updated json file with default value. Works now.

Add sorting to handle new work stealing.

Since not all custom window functions need the previous frame, push responsibility down to the function states that do. Pass around vectors so we have fewer silly arguments.

Factor out the frame intersection logic for reuse.

Fix the ModeState to have proper constructors and destructors and then call them.

Hack around memory leak in PerfectAggregateHashTable.

taniabogatsch

Hey @hawkfish! Thanks for the PR, EXCLUDE looks like a pretty neat extension to the windowing functionality. I added some comments and nits.

From my understanding, the significant change here is the switch from a single FrameBounds parameter to a vector of those? From what I can tell, we then determine what to exclude by setting the validity mask accordingly.

src/core_functions/aggregate/holistic/mode.cpp

test/sql/window/test_invalid_window.test

src/function/aggregate/distributive/count.cpp

third_party/libpg_query/grammar/statements/select.y

src/execution/window_executor.cpp

src/include/duckdb/common/vector_operations/aggregate_executor.hpp

src/execution/window_segment_tree.cpp

Incorporate review feedback. Most notably, the ValidityMask object is smart enough to handle the AllValid situation without constructing extra bitmasks.

taniabogatsch · 2023-10-09T08:41:24Z

Thanks for all the comments and explanations on my notes! LGTM now.

I mentioned the size_t stuff because of our C++ Guidelines written down here
As for using, it is personal preference, and I understand your reasoning, I was mostly commenting because I hadn't seen it much/anywhere else in our codebase yet! :)

Mytherin · 2023-10-16T07:15:09Z

Thanks!

Merge pull request duckdb/duckdb#9164 from Mause/feature/jdbc-uuid-param Merge pull request duckdb/duckdb#9185 from pdet/adbc_07 Merge pull request duckdb/duckdb#9126 from Maxxen/parquet-kv-metadata Merge pull request duckdb/duckdb#9123 from lnkuiper/parquet_schema Merge pull request duckdb/duckdb#9086 from lnkuiper/json_inconsistent_structure Merge pull request duckdb/duckdb#8977 from Tishj/python_readcsv_multi_v2 Merge pull request duckdb/duckdb#9279 from hawkfish/nsdate-cast Merge pull request duckdb/duckdb#8851 from taniabogatsch/binary_lambdas Merge pull request duckdb/duckdb#8983 from Maxxen/types/fixedsizelist Merge pull request duckdb/duckdb#9318 from Maxxen/fix-unused Merge pull request duckdb/duckdb#9220 from hawkfish/exclude Merge pull request duckdb/duckdb#9230 from Maxxen/json-plan-serialization Merge pull request duckdb/duckdb#9011 from Tmonster/add_create_statement_support_to_fuzzer Merge pull request duckdb/duckdb#9400 from Maxxen/array-fixes Merge pull request duckdb/duckdb#8741 from Tishj/python_import_cache_upgrade Merge fixes Merge pull request duckdb/duckdb#9395 from taniabogatsch/lambda-performance Merge pull request duckdb/duckdb#9427 from Tishj/python_table_support_replacement_scan Merge pull request duckdb/duckdb#9516 from carlopi/fixformat Merge pull request duckdb/duckdb#9485 from Maxxen/fix-parquet-serialization Merge pull request duckdb/duckdb#9388 from chrisiou/issue217 Merge pull request duckdb/duckdb#9565 from Maxxen/fix-array-vector-sizes Merge pull request duckdb/duckdb#9583 from carlopi/feature Merge pull request duckdb/duckdb#8907 from cryoEncryp/new-list-functions Merge pull request duckdb/duckdb#8642 from Virgiel/capi-streaming-arrow Merge pull request duckdb/duckdb#8658 from Tishj/pytype_optional Merge pull request duckdb/duckdb#9040 from Light-City/feature/set_mg

j-w-moebius and others added 30 commits July 19, 2023 12:28

add parser support for window EXCLUDE clauses

2b8b1d2

implement window EXCLUDE for aggregate functions

7df67c4

implement window EXCLUDE for first_value, last_value, nth_value

e280248

add tests for window EXCLUDE

7598dcb

add (de-)serialization of window exclusion clauses

09adb69

increment storage version

9297264

fix bug related to R API and window EXCLUDE

e0a6060

generate serialized_parsed_expression.cpp correctly

1bae6a0

fix parameter case style

1597ca9

undo unintended changes

da294f1

fix serialization mistakes

c0531a2

fix leaking EXCLUDE implementation for aggregates

245f5ff

add EXCLUDE tests for PARTITION BY without ORDER BY

82de0d6

remove unnecessary constructor argument

5b8e095

add tests for custom aggregates

87c0bf8

Merge branch 'master' into temp

da18305

minor fixes

724c1a1

Merge branch 'master' into exclude

627fb2b

Merge branch 'partition-order' into exclude

d1e4d7e

Merge branch 'partition-order' into exclude

5c2c274

Internal duckdb#202: EXCLUDE Result Order

23226f8

Reverse the expected order of one test now that we have deterministic ordering of single partitions.

Internal duckdb#202: Hide Exclude Evaluation

09b4012

Move the code for WindowSegmentTree::EvaluateInternal to WindowSegmentTreeState. This hides the implementation details and prepares to fix some memory leaks.

Internal duckdb#202: Right Memory Leak

0001d57

Fix the right side memory leak. Avoid unnecessary per-chunk memory allocation.

Internal duckdb#202: Custom Window Exclude

ef0ef10

Plumb the setting through and throw if it is not supported.

Merge branch 'partition-order' into exclude

f0deefd

Internal duckdb#202: Custom Window Exclude

ff60481

Change API to pass frame lists instead of window setting.

Internal duckdb#202: Count Star Exclude

0fe7e0f

Support the EXCLUDE modifiers for COUNT(*).

Internal duckdb#202: Constant Window Exclude

53eb947

Use swap instead of copying.

Merge branch 'partition-order' into exclude

9a6ffce

Merge branch 'main' into partition-order

f119650

Richard Wesley added 15 commits September 16, 2023 15:04

Internal duckdb#215: QUANTILE Reuse Refactor

fd03638

Convert ReuseIndexes to handle multiple ranges. Only tested for single range.

Merge branch 'main' into exclude

955251b

Internal duckdb#215: QUANTILE Reuse Restriction

596680d

Reduce the insert range of Reuse to the current frames instead of the union of current and previous frames.

Internal duckdb#215: QUANTILE Exclude Tests

65082ee

Back out range restriction. Add median tests.

Merge branch 'main' into exclude

7bd8778

Internal duckdb#202: Windowed Exclude Serialisation

7ab3086

Make the new exclude clause member optional so we don't have to bump the file version.

Merge branch 'feature' into exclude

24722ae

Internal duckdb#202: Windowed Exclude Serialisation

2b0d075

Still broken but should be closer. Probably just needs a `GetDefault`?

Merge branch 'feature' into exclude

269d61d

Internal duckdb#202: Windowed Exclude Serialisation

1514153

Updated json file with default value. Works now.

Internal duckdb#215: Deterministic EXCLUDE Tests

8422779

Add sorting to handle new work stealing.

Internal duckdb#358: Exclude API Polishing

ea29b8f

Since not all custom window functions need the previous frame, push responsibility down to the function states that do. Pass around vectors so we have fewer silly arguments.

Internal duckdb#358: Frame Intersection Template

0b5e433

Factor out the frame intersection logic for reuse.

Internal duckdb#358: MODE State Destructor

8ac840f

Fix the ModeState to have proper constructors and destructors and then call them.

Internal duckdb#358: MODE Memory Leak

ed90a1b

Hack around memory leak in PerfectAggregateHashTable.

hawkfish requested review from Mytherin and taniabogatsch October 4, 2023 15:59

taniabogatsch suggested changes Oct 5, 2023

View reviewed changes

Internal duckdb#215: Window EXCLUDE Feedback

bf8c8ab

Incorporate review feedback. Most notably, the ValidityMask object is smart enough to handle the AllValid situation without constructing extra bitmasks.

github-actions bot marked this pull request as draft October 5, 2023 23:40

taniabogatsch approved these changes Oct 9, 2023

View reviewed changes

Merge branch 'feature' into exclude

493368c

hawkfish marked this pull request as ready for review October 12, 2023 20:05

Mytherin merged commit e47be52 into duckdb:feature Oct 16, 2023
45 checks passed

hawkfish deleted the exclude branch October 16, 2023 14:22

Mytherin mentioned this pull request Nov 8, 2023

Support of EXCLUDE clauses for window functions #8311

Closed

lukaseder mentioned this pull request Feb 14, 2024

Upgrade DuckDB dialect to DuckDB 0.10.2 jOOQ/jOOQ#16287

Closed

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Internal #215: Window EXCLUDE Functionality #9220

Internal #215: Window EXCLUDE Functionality #9220

hawkfish commented Oct 4, 2023

taniabogatsch left a comment

taniabogatsch commented Oct 9, 2023

Mytherin commented Oct 16, 2023

Internal #215: Window EXCLUDE Functionality #9220

Internal #215: Window EXCLUDE Functionality #9220

Conversation

hawkfish commented Oct 4, 2023

taniabogatsch left a comment

Choose a reason for hiding this comment

taniabogatsch commented Oct 9, 2023

Mytherin commented Oct 16, 2023