Skip to content

Commit

Permalink
Merge #131254
Browse files Browse the repository at this point in the history
131254: opt: disallow non-standard NULL ordering with subqueries in test builds r=mgartner a=mgartner

The optimizer does not correctly build `ORDER BY` expressions with
non-standard `NULL` orderings when one or more of the ordering columns
is a projection containing a subquery. This causes test-only assertions
to fail in randomized tests. It is not yet known if there are any issues
with the results of these queries in production builds where the test
assertions are disabled.

For now, we simply return an "unimplemented" error to silence the
failures.

Informs #129956

Release note: None


Co-authored-by: Marcus Gartner <[email protected]>
  • Loading branch information
craig[bot] and mgartner committed Sep 25, 2024
2 parents e523823 + fe771d0 commit a699eb2
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 1 deletion.
4 changes: 3 additions & 1 deletion pkg/sql/explain_bundle_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,9 @@ CREATE TABLE users(id UUID DEFAULT gen_random_uuid() PRIMARY KEY, promo_id INT R
{"intervalstyle", "iso_8601"},
{"large_full_scan_rows", "2000"},
{"locality_optimized_partitioned_index_scan", "off"},
{"null_ordered_last", "on"},
// TODO(#129956): Enable this once non-default NULLS ordering with
// subqueries is allowed in tests.
// {"null_ordered_last", "on"},
{"on_update_rehome_row_enabled", "off"},
{"opt_split_scan_limit", "1000"},
{"optimizer_use_histograms", "off"},
Expand Down
2 changes: 2 additions & 0 deletions pkg/sql/opt/optbuilder/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ go_library(
importpath = "github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder",
visibility = ["//visibility:public"],
deps = [
"//pkg/build",
"//pkg/clusterversion",
"//pkg/kv/kvserver/concurrency/isolation",
"//pkg/server/telemetry",
Expand Down Expand Up @@ -96,6 +97,7 @@ go_library(
"//pkg/sql/syntheticprivilege",
"//pkg/sql/types",
"//pkg/util",
"//pkg/util/buildutil",
"//pkg/util/errorutil",
"//pkg/util/errorutil/unimplemented",
"//pkg/util/intsets",
Expand Down
34 changes: 34 additions & 0 deletions pkg/sql/opt/optbuilder/orderby.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ package optbuilder
import (
"fmt"

"github.com/cockroachdb/cockroach/pkg/build"
"github.com/cockroachdb/cockroach/pkg/server/telemetry"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
"github.com/cockroachdb/cockroach/pkg/sql/opt"
Expand All @@ -23,6 +24,8 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
"github.com/cockroachdb/cockroach/pkg/sql/types"
"github.com/cockroachdb/cockroach/pkg/util/buildutil"
"github.com/cockroachdb/errors"
)

// analyzeOrderBy analyzes an Ordering physical property from the ORDER BY
Expand Down Expand Up @@ -269,6 +272,12 @@ func (b *Builder) analyzeExtraArgument(
// Ensure we can order on the given column(s).
ensureColumnOrderable(e)
if !nullsDefaultOrder {
if buildutil.CrdbTestBuild && containsSubquery(e) {
panic(errors.UnimplementedError(
errors.IssueLink{IssueURL: build.MakeIssueURL(129956)},
"subqueries in ORDER BY with non-default NULLS ordering is not supported"),
)
}
metadataName := fmt.Sprintf("nulls_ordering_%s", e.String())
extraColsScope.addColumn(
scopeColName("").WithMetadataName(metadataName),
Expand All @@ -279,6 +288,31 @@ func (b *Builder) analyzeExtraArgument(
}
}

func containsSubquery(expr tree.Expr) bool {
var v subqueryVisitor
tree.WalkExprConst(&v, expr)
return v.foundSubquery
}

type subqueryVisitor struct {
foundSubquery bool
}

var _ tree.Visitor = &subqueryVisitor{}

func (s *subqueryVisitor) VisitPre(expr tree.Expr) (recurse bool, newExpr tree.Expr) {
if s.foundSubquery {
return false, expr
}
if _, ok := expr.(*subquery); ok {
s.foundSubquery = true
return false, expr
}
return true, expr
}

func (s *subqueryVisitor) VisitPost(expr tree.Expr) tree.Expr { return expr }

// hasDefaultNullsOrder returns whether the provided ordering uses the default
// ordering for NULLs. The default order in Cockroach if null_ordered_last=False
// is nulls first for ascending order and nulls last for descending order.
Expand Down
15 changes: 15 additions & 0 deletions pkg/sql/opt/optbuilder/testdata/orderby
Original file line number Diff line number Diff line change
Expand Up @@ -1265,3 +1265,18 @@ sort
├── columns: a:1!null b:2 c:3 d:4
└── scan abcd
└── columns: a:1!null b:2 c:3 d:4 crdb_internal_mvcc_timestamp:5 tableoid:6

# TODO(#129956): Handle non-default NULL ordering that references a projected
# expression that contains a subquery.
exec-ddl
CREATE TABLE t129956 (i INT)
----

build
SELECT * FROM t129956 AS t0
WHERE EXISTS(
SELECT (SELECT t0.i FROM t129956) AS tmp
ORDER BY tmp NULLS LAST
)
----
error (0A000): subqueries in ORDER BY with non-default NULLS ordering is not supported

0 comments on commit a699eb2

Please sign in to comment.