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: do not inline strict UDFs #95240

Closed
mgartner opened this issue Jan 13, 2023 · 0 comments · Fixed by #94797
Closed

sql: do not inline strict UDFs #95240

mgartner opened this issue Jan 13, 2023 · 0 comments · Fixed by #94797
Assignees
@mgartner
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-queries SQL Queries Team

Comments

@mgartner
Copy link
Collaborator

mgartner commented Jan 13, 2023 •
edited by cockroach-jira-scripts
Loading

On master, inlining of strict UDFs produces incorrect results:

defaultdb> CREATE FUNCTION fn(i INT) RETURNS INT STRICT
        -> IMMUTABLE LANGUAGE SQL AS 'SELECT 33';
CREATE FUNCTION

defaultdb> CREATE TABLE t (a INT);
CREATE TABLE

defaultdb> INSERT INTO t VALUES (1), (NULL);
INSERT 0 2

defaultdb> SELECT fn(a) FROM t;
  fn
------
  33
  33
(2 rows)

One of the rows returned should be NULL.

Jira issue: CRDB-23417
@mgartner mgartner added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Jan 13, 2023
@blathers-crl blathers-crl bot added the T-sql-queries SQL Queries Team label Jan 13, 2023
@mgartner mgartner self-assigned this Jan 13, 2023
mgartner added a commit to mgartner/cockroach that referenced this issue Jan 13, 2023
@mgartner
sql: implement strict UDFs with CASE
57ed4ed 
This commit removes the field `CalledOnNullInput` of `tree.RoutineExpr`.
Strict UDFs are now wrapped in a `CASE` expression so that they are not
invoked when any of the arguments are NULL.

This fixes a bug that caused strict functions be evaluated when
presented with `NULL` arguments after being inlined. This also allows
for the removal of special logic in evaluation of `tree.RoutineExpr`.

A positive side effect of this change is that strict UDFs will be folded
to NULL at optimization-time if they are presented with constant NULL
arguments. This is made possible by existing normalization rules.

Fixes cockroachdb#95240

There is no release note because the bug is not present in any releases.

Release note: None
@mgartner mgartner mentioned this issue Jan 13, 2023
Merged
mgartner added a commit to mgartner/cockroach that referenced this issue Jan 18, 2023
@mgartner
sql: implement strict UDFs with CASE
d7dbfb9 
This commit removes the field `CalledOnNullInput` of `tree.RoutineExpr`.
Strict UDFs are now wrapped in a `CASE` expression so that they are not
invoked when any of the arguments are NULL.

This fixes a bug that caused strict functions be evaluated when
presented with `NULL` arguments after being inlined. This also allows
for the removal of special logic in evaluation of `tree.RoutineExpr`.

A positive side effect of this change is that strict UDFs will be folded
to NULL at optimization-time if they are presented with constant NULL
arguments. This is made possible by existing normalization rules.

Fixes cockroachdb#95240

There is no release note because the bug is not present in any releases.

Release note: None
craig bot pushed a commit that referenced this issue Jan 19, 2023
@gtr @mgartner @andyyang890
Merge #94002 #94797 #95382
9735954 
94002: ui: add missing filters for workload insights and active execution pages r=gtr a=gtr

Fixes #87741, #87783.

Previously, the workload insights page was missing a filter for insights
type for both the statement and transaction tabs. Furthermore, the
active execution pages for statement and transactions was also missing a
status filter.

This commit adds the ability to filter by insights type for the workload
insights page and the ability to filter by execution status for the
active execution pages.

DB Console video demos:
- [Demo](https://www.loom.com/share/39c7b882b8b84ebfb39203d219f05075) for workload insights page.
- [Demo](https://www.loom.com/share/806c1238233741e2b17882e3665ffabf) for active execution page.
- [Demo](https://www.loom.com/share/e601c39c20cb45b28aae5ac468216ba6) for selecting more than one option.

CC Console video demos
- [Demo](https://www.loom.com/share/975b0952a43d4ab6908cbb5a0533f335) for filtering by execution status on statements tab
- [Demo](https://www.loom.com/share/f60690b61c8d4665b87b0f31f639596c) for filtering by execution status on transactions tab
- [Demo](https://www.loom.com/share/13cd1865edcb4b8d87ba7b89217e697d) for filtering by workload insight type

Release note (ui change): added an insights type filter for the workload
insights page, added a execution status filter for the active execution
pages. Updated the tooltip for execution status to include the definition
for the "Waiting" status.

94797: sql: implement strict UDFs with CASE r=mgartner a=mgartner

This commit removes the field `CalledOnNullInput` of `tree.RoutineExpr`.
Strict UDFs are now wrapped in a `CASE` expression so that they are not
invoked when any of the arguments are NULL.

This fixes a bug that caused strict functions be evaluated when
presented with `NULL` arguments after being inlined. This also allows
for the removal of special logic in evaluation of `tree.RoutineExpr`.

A positive side effect of this change is that strict UDFs will be folded
to NULL at optimization-time if they are presented with constant NULL
arguments. This is made possible by existing normalization rules.

Fixes #95240

There is no release note because the bug is not present in any releases.

Release note: None


95382: upgrades_test: speed up TestRoleMembersIDMigration1500Users test r=rafiss a=andyyang890

This patch speeds up the TestRoleMembersIDMigration1500Users test by
moving the creation of test users into transactions. On my Mac, this
change reduced the running time from ~55s to ~15s (a 4x speedup).

Fixes #95268

Release note: None

Co-authored-by: gtr <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Andy Yang <[email protected]>
@craig craig bot closed this as completed in c2c6239 Jan 19, 2023
@mgartner mgartner moved this to Done in SQL Queries Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mgartner mgartner

Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-queries SQL Queries Team
Projects
SQL Queries
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql: implement strict UDFs with CASE mgartner/cockroach
1 participant
@mgartner