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

opt: tuple star expansion is incorrect for unnamed tuples #48179

Closed
rytaft opened this issue Apr 29, 2020 · 3 comments · Fixed by #48225
Closed

opt: tuple star expansion is incorrect for unnamed tuples #48179

rytaft opened this issue Apr 29, 2020 · 3 comments · Fixed by #48225
Labels
A-sql-optimizer SQL logical planning and optimizations. good first issue

Comments

@rytaft
Copy link
Collaborator

rytaft commented Apr 29, 2020

The following two queries are equivalent except for the fact that the first one involves unnamed tuple elements, while the second one has named elements. However, the plans are different:

[email protected]:50033/movr> explain (opt) select (b).* from (values (((1, 2)))) as a(b);
           text
--------------------------
  project
   ├── values
   │    └── ((1, 2),)
   └── projections
        └── (column1).@1
(5 rows)

[email protected]:50033/movr> explain (opt) select (b).* from (values (((1, 2) as x,y))) as a(b);
              text
---------------------------------
  project
   ├── values
   │    └── (((1, 2) AS x, y),)
   └── projections
        ├── (column1).x
        └── (column1).y
(6 rows)

Notice that the first plan only projects the first element of the tuple, not the second. As a result, executing that plan produces the following erroneous output:

[email protected]:50033/movr>  select (b).* from (values (((1, 2)))) as a(b);
  ?column? | ?column?
-----------+-----------
         1 |        1
(1 row)

The second plan with named tuple elements works as expected:

[email protected]:50033/movr> select (b).* from (values (((1, 2) as x,y))) as a(b);
  x | y
----+----
  1 | 2
(1 row)
@rytaft rytaft added the A-sql-optimizer SQL logical planning and optimizations. label Apr 29, 2020
@DrewKimball
Copy link
Collaborator

Changing line 88 in pkg/sql/opt/optbuilder/util.go from this:

aliases = append(aliases, "?column?")

to this:

aliases = append(aliases, fmt.Sprintf("%s.@%d", "?column?", i+1))

seems to fix the issue. The problem is that when a tuple has no labels, every tuple field is assigned the same alias. All the new ColumnAccess expressions end up pointing to the first tuple field.

@rytaft
Copy link
Collaborator Author

rytaft commented Apr 30, 2020

Nice find! But looking closer here I think the problem is that NewTypedColumnAccessExpr expects colName to be empty when there is no name and it should be accessed by index. So I think we might actually want to move the line aliases = append(aliases, "?column?") down below the call to NewTypedColumnAccessExpr. Feel free to submit a PR to fix it or I can get to it tomorrow. Thanks!

@rytaft
Copy link
Collaborator Author

rytaft commented Apr 30, 2020

@DrewKimball I went ahead an opened a PR since I'm not sure I described the fix very well. Thanks for helping debug this!

craig bot pushed a commit that referenced this issue Apr 30, 2020
48163: schemachange: more informative dependent job descriptions r=spaskob a=spaskob

`DROP xxx` statements have cascading effects of causing sequences, indexes
and views being dropped as well. We should name the jobs that execute them
accordingly so that it is clear to users that they are connected to the
inital `DROP` statement.

Fixes #35051.

Release note (bug fix): Better distinguish between the delete jobs for
columns and dependent jobs for deleting indices, views and sequences. In the
current state clients were confused why more than 1 job was created from
a `DROP COLUMN` statement.

48216: sem/builtins: avoid an assertion error on legitimate errors r=yuzefovich a=knz

Fixes #46035

If there's a retry error or some other legitimate txn error
while checking a privilege, this will come up during the privilege
check and should flow to the client. No need to issue an "internal
error" and a sentry report.

Release note (bug fix): CockroachDB will now avoid producing a severe
"internal error" upon certain privilege check failures via
`pg_catalog` built-in functions.

48225: opt: fix star expansion of un-labeled tuples r=rytaft a=rytaft

Prior to this commit, there was a bug with star expansion of
un-labeled tuples in which only the first column of the tuple
was projected. This was happening because the name "?column?"
was being passed to `NewTypedColumnAccessExpr` for all columns in
the tuple, which made it impossible to determine that they were
actually different expressions. This commit fixes the issue by
ensuring that an empty column name is always passed to
`NewTypedColumnAccessExpr` when the tuple elements are unlabeled.
This signals that the tuple index should be used instead of the
name.

Fixes #48179

Release note (bug fix): Fixed a bug in which (tuple).* was only
expanded to the first column in the tuple and the remaining
elements were dropped.

48236: kv: cleanup a few trace events during replica evaluation r=nvanbenschoten a=nvanbenschoten

Removes a useless trace event and adds two helpful ones in.

48246: kvclient: properly relax assertion about txn already committed r=andreimatei a=andreimatei

I tried to relax it just before, but lefta fall-through to another
fatal.

Release note: None

Co-authored-by: Spas Bojanov <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Rebecca Taft <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Andrei Matei <[email protected]>
@craig craig bot closed this as completed in a30db4d Apr 30, 2020
rytaft added a commit to rytaft/cockroach that referenced this issue May 1, 2020
Prior to this commit, there was a bug with star expansion of
un-labeled tuples in which only the first column of the tuple
was projected. This was happening because the name "?column?"
was being passed to NewTypedColumnAccessExpr for all columns in
the tuple, which made it impossible to determine that they were
actually different expressions. This commit fixes the issue by
ensuring that an empty column name is always passed to
NewTypedColumnAccessExpr when the tuple elements are unlabeled.
This signals that the tuple index should be used instead of the
name.

Fixes cockroachdb#48179

Release note (bug fix): Fixed a bug in which (tuple).* was only
expanded to the first column in the tuple and the remaining
elements were dropped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-optimizer SQL logical planning and optimizations. good first issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants