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.

Sign up for GitHub

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

Jump to bottom

sql/tests: TestRandomSyntaxFuncCommon failed [nil pointer in optbuilder] #109069

Closed
cockroach-teamcity opened this issue Aug 19, 2023 · 5 comments · Fixed by #109861
Closed

sql/tests: TestRandomSyntaxFuncCommon failed [nil pointer in optbuilder] #109069

cockroach-teamcity opened this issue Aug 19, 2023 · 5 comments · Fixed by #109861
Assignees
@rharding6373
Labels
branch-master Failures and bugs on the master branch. C-test-failure Broken test (automatically or manually discovered). O-robot Originated from a bot. O-rsg Random Syntax Generator T-sql-queries SQL Queries Team X-noreuse Prevent automatic commenting from CI test failures
Milestone
23.2

Comments

@cockroach-teamcity
Copy link
Member

cockroach-teamcity commented Aug 19, 2023 •
edited by cockroach-jira-scripts
Loading

sql/tests.TestRandomSyntaxFuncCommon failed with artifacts on master @ 9fcc5c8ce60d2f25e06e41bd97e20445233f9fa5:

Random syntax error:

    rsg_test.go:880: Crash detected: server panic: pq: internal error: runtime error: invalid memory address or nil pointer dereference

Query:

        SELECT TRIM ( BOTH FROM [ FUNCTION 261 ] ( * ) WITHIN GROUP ( ORDER BY INDEX_AFTER_ORDER_BY_BEFORE_AT INT . LIKE @ FAMILY ) );
Help

See also: How To Investigate a Go Test Failure (internal)

/cc @cockroachdb/sql-foundations

This test on roachdash | Improve this report!

Jira issue: CRDB-30764
@cockroach-teamcity cockroach-teamcity added branch-master Failures and bugs on the master branch. C-test-failure Broken test (automatically or manually discovered). O-robot Originated from a bot. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Aug 19, 2023
@cockroach-teamcity cockroach-teamcity added this to the 23.2 milestone Aug 19, 2023
@rafiss rafiss changed the title sql/tests: TestRandomSyntaxFuncCommon failed sql/tests: TestRandomSyntaxFuncCommon failed [nil pointer in optbuilder] Aug 21, 2023
@rafiss rafiss added the X-noreuse Prevent automatic commenting from CI test failures label Aug 21, 2023
@rafiss
Copy link
Collaborator

rafiss commented Aug 21, 2023

Reproduces on a fresh cluster using:

SELECT TRIM ( BOTH FROM [ FUNCTION 261 ] ( * ) WITHIN GROUP ( ORDER BY INDEX_AFTER_ORDER_BY_BEFORE_AT INT . LIKE @ FAMILY ) );

@chrisseto
Copy link
Contributor

@rafiss need a volunteer for this one?

@rafiss
Copy link
Collaborator

rafiss commented Aug 21, 2023

I'll move this over to SQL Queries, since it relates to the optimizer code.

@rafiss rafiss added T-sql-queries SQL Queries Team and removed T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Aug 21, 2023
@github-project-automation github-project-automation bot moved this to Triage in SQL Queries Aug 21, 2023
@rharding6373 rharding6373 self-assigned this Aug 23, 2023
@rharding6373 rharding6373 moved this from Triage to Active in SQL Queries Aug 23, 2023
@mgartner mgartner added the O-rsg Random Syntax Generator label Aug 25, 2023
@rharding6373
Copy link
Collaborator

Slightly simpler repro:

CREATE TABLE tbl (a int);
SELECT percentile_disc ( 0.50 ) WITHIN GROUP ( ORDER BY PRIMARY KEY tbl ) FROM tbl;

I spent some time looking at this last week. So far what I know is that when we build the scope, in replaceAggregate we expect to have an expression for the ORDER BY. This is used to resolve the percentile_disc function. However, PRIMARY KEY and INDEX_AFTER_ORDER_BY_BEFORE_AT do not have expressions from parsing, which means no overload for the function is found.

Some options I would consider:

  1. Disable the PRIMARY KEY and INDEX_AFTER_ORDER_BY_BEFORE_AT for this limited situation (used in an aggregate function). This is CRDB only syntax.
  2. See if we could use the catalog to build an equivalent expression in this case.

@mgartner
Copy link
Collaborator

(1) seems like the best option for now. No point in adding this special syntax without a known use case for it.

@rharding6373 rharding6373 mentioned this issue Aug 31, 2023
Merged
craig bot pushed a commit that referenced this issue Sep 5, 2023
@rharding6373 @kvoli @renatolabs
Merge #109861 #110016 #110029
71eaeec 
109861: sql: disable order by index in aggregate decoration clauses r=rharding6373 a=rharding6373

Before this change, queries like `SELECT percentile_disc ( 0.50 ) WITHIN GROUP ( ORDER BY PRIMARY KEY tbl ) FROM tbl;` would fail with an internal error. This is due to optbuilder expecting an order expression, which order by index does not provide, in the aggregate case in order to resolve the function.

Since this functionality has never worked and appears to be a rare or never used feature, this PR disables queries with order by index in this position at the parsing stage. Issue #109847 has been opened to track usage.

Epic: None
Fixes: #109069
Informs: #109847

Release note: None.

110016: github: split up spanconfig(ccl) owners  r=arulajmani,rafiss a=kvoli

Prior to this commit, `kv-prs` was the owner of `pkg/spanconfig` and
`pkg/ccl/spanconfigccl`. Package ownership is updated to reflect the
ownership split between `kv-prs` and `sql-foundations`.

See CODEOWNERS for the split.

Epic: none
Release note: None

110029: roachtest: fix calls to schemachange workload in acceptance/version-upgrade r=fqazi a=renatolabs

The `schemachange` workload would always be called in that test; however, we are currently only staging the `workload` binary build on the SHA being tested; this is insufficient now that we test multiple upgrades in `mixedversion` tests.

We fix the immediate issue by not invoking the workload when we are not performing an upgrade or downgrade involving the current cockroach binary. In the future, it would be nice to stage multiple workload binaries and use the appropriate one in the mixed-version hook.

Epic: none

Release note: None

Co-authored-by: rharding6373 <[email protected]>
Co-authored-by: Austen McClernon <[email protected]>
Co-authored-by: Renato Costa <[email protected]>
@craig craig bot closed this as completed in 0deff4c Sep 5, 2023
@github-project-automation github-project-automation bot moved this from Active to Done in SQL Queries Sep 5, 2023
blathers-crl bot pushed a commit that referenced this issue Oct 5, 2023
@rharding6373
sql: disable order by index in aggregate decoration clauses
bd21e67 
Before this change, queries like `SELECT percentile_disc ( 0.50 ) WITHIN
GROUP ( ORDER BY PRIMARY KEY tbl ) FROM tbl;` would fail with an
internal error. This is due to optbuilder expecting an order expression,
which order by index does not provide, in the aggregate case in order to
resolve the function.

Since this functionality has never worked and appears to be a rare or
never used feature, this PR disables queries with order by index in this
position at the parsing stage. Issue #109847 has been opened to track
usage.

Epic: None
Fixes: #109069
Informs: #109847

Release note: None.
@blathers-crl blathers-crl bot mentioned this issue Oct 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rharding6373 rharding6373

Labels
branch-master Failures and bugs on the master branch. C-test-failure Broken test (automatically or manually discovered). O-robot Originated from a bot. O-rsg Random Syntax Generator T-sql-queries SQL Queries Team X-noreuse Prevent automatic commenting from CI test failures
Projects
SQL Queries
Archived in project
Milestone
23.2
Development

Successfully merging a pull request may close this issue.

sql: disable order by index in aggregate decoration clauses rharding6373/cockroach
5 participants
@mgartner @rafiss @chrisseto @rharding6373 @cockroach-teamcity