sql: reject WITHIN GROUP on non-ordered-set aggregates

[virajchogle]

Fixes #171236

WITHIN GROUP (ORDER BY ...) is only valid on ordered-set aggregate functions (percentile_disc, percentile_cont). CockroachDB silently ignored the clause on general aggregates (sum, avg, ...) and non-aggregate scalar functions (concat_ws, abs, ...), diverging from PostgreSQL's behavior.

This rejects the misuse in scope.VisitPre against the isOrderedSetAggregate allowlist, using pgcode.WrongObjectType (SQLSTATE 42809) to match PostgreSQL.

Epic: none

Release note (bug fix): WITHIN GROUP on a function that is not an ordered-set aggregate now returns an error matching PostgreSQL.

[trunk-io]

Merging to master in this repository is managed by Trunk.

• To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

[blathers-crl]

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[michae2]

Hi @virajchogle, thanks for the PR!

It's not showing up here, but there are some test failures. For example, here's one you can play with locally:

./dev testlogic base --config=3node-tenant --files=enums

The failure has the error function array_agg is not an ordered set aggregate, here's one example failure:

==================== Test output for //pkg/sql/logictest/tests/3node-tenant:3node-tenant_test:
metamorphic: use COCKROACH_RANDOM_SEED=3173743833643951676 for reproduction
initialized metamorphic constant "max-batch-byte-size" with value 17240577
initialized metamorphic constant "parse-json-impl" with value 1
initialized metamorphic constant "coldata-batch-size" with value 2287
initialized metamorphic constant "span-reuse-rate" with value 62
initialized metamorphic constant "kv.closed_timestamp.lead_for_global_reads_auto_tune.enabled" with value true
initialized metamorphic constant "default-batch-bytes-limit" with value 41805
initialized metamorphic constant "logical_replication.consumer.immediate_mode_writer" with value crud
initialized metamorphic constant "kvadmission.flow_control.mode" with value apply_to_all
initialized metamorphic constant "max-intents-in-flight-per-caller" with value 173
initialized metamorphic constant "distsender-leaseholder-first" with value false
initialized metamorphic constant "kv.transaction.randomized_anchor_key.enabled" with value true
initialized metamorphic constant "kv.transaction.write_buffering.enabled" with value false
initialized metamorphic constant "kv.transaction.write_buffering.max_buffer_size" with value 2579
initialized metamorphic constant "spilling-queue-initial-len" with value 15
initialized metamorphic constant "merge-joiner-groups-buffer" with value 1
initialized metamorphic constant "lock-table-iters-before-seek" with value 0
initialized metamorphic constant "mvcc-incremental-iter-tbi" with value false
initialized metamorphic constant "mvcc-value-disable-simple-encoding" with value true
initialized metamorphic constant "storage.ingest_split.enabled" with value false
initialized metamorphic constant "storage.delete_compaction_excise.enabled" with value false
initialized metamorphic constant "storage.value_separation.enabled" with value false
initialized metamorphic constant "storage.value_separation.compaction_garbage_threshold" with value 15
initialized metamorphic constant "storage.value_separation.compaction_garbage_threshold" with value 29
initialized metamorphic constant "storage.value_separation.mvcc_history_minimum_size" with value 146061
initialized metamorphic constant "mvcc-max-iters-before-seek" with value 0
initialized metamorphic constant "disable-checksstconflicts-range-key-masking" with value true
initialized metamorphic constant "storage.ingestion.value_blocks.enabled" with value false
initialized metamorphic constant "raft-log-truncation-clearrange-threshold" with value 5931
initialized metamorphic constant "kv.lock_table.unreplicated_lock_reliability.split.enabled" with value false
initialized metamorphic constant "kv.lock_table.unreplicated_lock_reliability.merge.enabled" with value false
initialized metamorphic constant "kv.concurrency.virtual_intent_resolution.enabled" with value true
initialized metamorphic constant "addsst-rewrite-concurrency" with value 15
initialized metamorphic constant "split-scans-right-for-stats-first" with value true
initialized metamorphic constant "kv.raft_log.loosely_coupled_truncation.enabled" with value true
initialized metamorphic constant "kv.snapshot.ingest_as_write_threshold" with value 102400
initialized metamorphic constant "kv.lease.expiration_leases_only.enabled" with value true
initialized metamorphic constant "kv.rangefeed.buffered_sender.enabled" with value false
initialized metamorphic constant "rangefeed-bulk_delivery" with value 2976778
initialized metamorphic constant "kv.raft.leader_fortification.fraction_enabled" with value 0
initialized metamorphic constant "disable-catalog-leased-descriptors-threshold" with value 64
initialized metamorphic constant "changefeed.bulk_delivery.enabled" with value false
initialized metamorphic constant "changefeed.resolved_timestamp.granularity" with value 2
initialized metamorphic constant "changefeed.progress.per_table_tracking.enabled" with value false
initialized metamorphic constant "sql.hints.statement_hints_cache_size" with value 128
initialized metamorphic constant "hint-cache-rangefeed-buffer-size" with value 2
initialized metamorphic constant "row-delete-range-chunk-size" with value 26
initialized metamorphic constant "datum-row-converter-batch-size" with value 40
initialized metamorphic constant "ColIndexJoin-using-streamer-batch-size" with value 8010187
initialized metamorphic constant "parallel-scan-result-threshold" with value 3865
initialized metamorphic constant "bulkio.index_backfill.distributed_merge.mode" with value disabled
initialized metamorphic constant "inverted-joiner-batch-size" with value 2
initialized metamorphic constant "zig-zag-joiner-batch-size" with value 4
initialized metamorphic constant "sql.txn.write_buffering_for_weak_isolation.enabled" with value true
initialized metamorphic constant "conn-executor-force-exec-with-pausable-portal" with value true
initialized metamorphic constant "copy-batch-size" with value 13280
initialized metamorphic constant "use-index-lookup-for-descriptors-in-database" with value false
initialized metamorphic constant "copy-fast-path-enabled-default" with value false
initialized metamorphic constant "multiple_active_portals_enabled" with value true
initialized metamorphic constant "backup-presplit-spans" with value false
initialized metamorphic constant "export_request_verbose_tracing" with value true
initialized metamorphic constant "logical_replication.consumer.try_optimistic_insert.enabled" with value false
initialized metamorphic constant "import-row-count-validation" with value sync
initialized metamorphic constant "default_range_distribution_strategy" with value balanced_simple
initialized metamorphic constant "changefeed.parallel_io.request_quota" with value 256
initialized metamorphic constant "changefeed.new_kafka_sink.enabled" with value false
initialized metamorphic constant "logictest-disable-opt-rule-probability" with value 1
initialized metamorphic constant "logictest-optimizer-cost-perturbation" with value 1
initialized metamorphic constant "logictest-use-mvcc-range-tombstones-for-point-deletes" with value true
I260609 15:35:20.841038 1 (gostd) rand.go:288  [-] 1  random seed: 3173743833643951676
test logs left over in: /tmp/cockroach/_tmp/4d1f1d9b51166205e76a7876ac26e6de/logTestLogic_enums871571080
--- FAIL: TestLogic_enums (9.93s)
    test_log_scope.go:172: test logs captured to: /tmp/cockroach/_tmp/4d1f1d9b51166205e76a7876ac26e6de/logTestLogic_enums871571080
    test_log_scope.go:82: use -show-logs to present logs inline
    logictestbase.go:583: statement/query skipped with reason: unsupported configuration 3node-tenant, statement/query only supports schema-locked-disabled (no issue given)
    logictestbase.go:583: statement/query skipped with reason: unsupported configuration 3node-tenant, statement/query only supports schema-locked-disabled (no issue given)
    logictestbase.go:583: statement/query skipped with reason: unsupported configuration 3node-tenant (no issue given)
    logictestbase.go:583: statement/query skipped with reason: unsupported configuration 3node-tenant, statement/query only supports schema-locked-disabled (no issue given)
    logictestbase.go:583: statement/query skipped with reason: unsupported configuration 3node-tenant, statement/query only supports schema-locked-disabled (no issue given)
    logictestbase.go:583: statement/query skipped with reason: unsupported configuration 3node-tenant, statement/query only supports schema-locked-disabled (no issue given)
    --- FAIL: TestLogic_enums/schema_changes (5.95s)
        logictestbase.go:583: statement/query skipped with reason: unsupported configuration 3node-tenant, statement/query only supports local-legacy-schema-changer (no issue given)
        logic.go:3430:

            /private/var/tmp/_bazel_michae2/682ad596a6ce446a7b1b3701799b649f/sandbox/darwin-sandbox/9473/execroot/com_github_cockroachdb_cockroach/bazel-out/darwin_arm64-fastbuild/bin/pkg/sql/logictest/tests/3node-tenant/3node-tenant_test_/3node-tenant_test.runfiles/com_github_cockroachdb_cockroach/pkg/sql/logictest/testdata/logic_test/enums:1289: SELECT * FROM [SHOW ENUMS] ORDER BY name
            expected success, but found
            (42809) function array_agg is not an ordered set aggregate
            scope.go:1168: in VisitPre()
        logic.go:2551:
             /private/var/tmp/_bazel_michae2/682ad596a6ce446a7b1b3701799b649f/sandbox/darwin-sandbox/9473/execroot/com_github_cockroachdb_cockroach/bazel-out/darwin_arm64-fastbuild/bin/pkg/sql/logictest/tests/3node-tenant/3node-tenant_test_/3node-tenant_test.runfiles/com_github_cockroachdb_cockroach/pkg/sql/logictest/testdata/logic_test/enums:1305: too many errors encountered, skipping the rest of the input
    logic.go:5054: -- test log scope end --
FAIL
I260609 15:35:30.777378 1 (gostd) testmain.go:976  [-] 1  Test //pkg/sql/logictest/tests/3node-tenant:3node-tenant_test exited with error code 1
================================================================================
INFO: Found 1 test target...
Target //pkg/sql/logictest/tests/3node-tenant:3node-tenant_test up-to-date:
  _bazel/bin/pkg/sql/logictest/tests/3node-tenant/3node-tenant_test_/3node-tenant_test
INFO: Elapsed time: 11.627s, Critical Path: 10.44s
INFO: 548 processes: 546 remote cache hit, 1 internal, 1 darwin-sandbox.
INFO: Build completed, 1 test FAILED, 548 total actions
//pkg/sql/logictest/tests/3node-tenant:3node-tenant_test                 FAILED in 10.2s
  /private/var/tmp/_bazel_michae2/682ad596a6ce446a7b1b3701799b649f/execroot/com_github_cockroachdb_cockroach/bazel-out/darwin_arm64-fastbuild/testlogs/pkg/sql/logictest/tests/3node-tenant/3node-tenant_test/test.log

Executed 1 out of 1 test: 1 fails locally.
ERROR: exit status 3

You can also see this error if you start up cockroach demo and run the statement SELECT * FROM [SHOW ENUMS] ORDER BY name;, for example:

./dev build short
./cockroach demo -e 'SELECT * FROM [SHOW ENUMS] ORDER BY name'

[virajchogle]

Thanks for catching this @michae2 . Root cause is the SHOW ENUMS delegate (pkg/sql/delegate/show_enums.go:42), which uses array_agg(...) WITHIN GROUP (ORDER BY ...), the very syntax this PR rejects. Switching to array_agg(... ORDER BY ...). Tested. Pushing shortly.

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
• When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}