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.

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

sql: Make USING EXTREMES predicates parsable #92792

Conversation

faizaanmadhani
Copy link
Contributor

@faizaanmadhani faizaanmadhani commented Dec 1, 2022

Previously the USING EXTREMES predicates
were constructed as strings which would
make them very difficult to parse. This PR
changes the way these predicates are
constructed so that they can later be parsed.

Release note (sql change): USING EXTREME predicates in the output of
SHOW STATISTICS will have additional
parenthesis and type assertions.

Epic: CRDB-19449

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@faizaanmadhani faizaanmadhani marked this pull request as ready for review December 1, 2022 05:42
@faizaanmadhani faizaanmadhani requested a review from a team as a code owner December 1, 2022 05:43
lbExpr := tree.ComparisonExpr{
Operator: treecmp.MakeComparisonOperator(treecmp.LT),
Left: &tree.ColumnItem{ColumnName: tree.Name(columnName)},
Right: extremesSpans.Get(0).EndKey().Value(0),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Both if/else cases look identical except for the span values. You could simplify things by just setting the two span values to variable in if/else and moving the expression tree construction outside the if/else.

nit: Some decomposition of constructUsingExtremesSpans might simplify things further so you don't have to reach into spans here. You could have a function that returns the histogram upper and lower bound, and pass those to both constructUsingExtremesSpans and constructUsingExtremesPredicate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@faizaanmadhani faizaanmadhani force-pushed the faizaan/modify-predicate-serializing branch 2 times, most recently from 8e62759 to 1b50af0 Compare December 6, 2022 15:23
Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Thank you for doing this!

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @faizaanmadhani and @mgartner)


-- commits line 12 at r2:
nit: We haven't released any version with USING EXTREMES yet, so I don't think you need a release note for this.


pkg/sql/distsql_plan_stats.go line 561 at r2 (raw file):

			Left:  &ubExpr,
			Right: &nullExpr,
		},

nit: This is total bikeshedding, but could we put the nullExpr to the left of the lbExpr? That better reflects what is happening (a [/ - /foo] span reads from the nulls first)

@faizaanmadhani faizaanmadhani force-pushed the faizaan/modify-predicate-serializing branch 2 times, most recently from 3317fcf to 2503273 Compare December 7, 2022 07:47
@faizaanmadhani
Copy link
Contributor Author

TFTRs! 🎉

bors r=michae2

@craig
Copy link
Contributor

craig bot commented Dec 7, 2022

Build failed:

@faizaanmadhani
Copy link
Contributor Author

bors r-

Previously the USING EXTREMES predicates
were constructed as strings which would
make them very difficult to parse. This PR
changes the way these predicates are
constructed so that they can later be parsed.

Release note: None

Epic: CRDB-19449
@faizaanmadhani faizaanmadhani force-pushed the faizaan/modify-predicate-serializing branch from 2503273 to 495b4cb Compare December 7, 2022 08:09
@faizaanmadhani
Copy link
Contributor Author

bors r=michae2

@craig
Copy link
Contributor

craig bot commented Dec 7, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Dec 7, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Dec 7, 2022

Build succeeded:

@craig craig bot merged commit a9fcbd1 into cockroachdb:master Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants