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

mirror: remove unnecessary shadowing #1609

Merged
merged 1 commit into from Feb 3, 2020
Merged

Conversation

wchargin
Copy link
Member

@wchargin wchargin commented Feb 2, 2020

Summary:
A common table expression shadows a (main or temporary) table name for
SELECT statements, but doing so is confusing and makes the code harder
to read.

Test Plan:
Existing unit tests suffice.

wchargin-branch: mirror-no-cte-shadow

Summary:
A common table expression shadows a (main or temporary) table name for
`SELECT` statements, but doing so is confusing and makes the code harder
to read.

Test Plan:
Existing unit tests suffice.

wchargin-branch: mirror-no-cte-shadow
@wchargin
Copy link
Member Author

wchargin commented Feb 2, 2020

FWIW, I’m especially inclined to patch this because I wasn’t able to
find an official source on whether it’s well defined behavior. I asked
on the sqlite-users list, but haven’t heard back yet:
http://sqlite.1065341.n5.nabble.com/Shadowing-a-table-name-with-a-common-table-expression-td110832.html

@Beanow
Copy link
Contributor

Beanow commented Feb 2, 2020

Needed to look a bit closer for what shadowing you meant.

db.prepare(
dedent`\
CREATE TEMPORARY TABLE transitive_dependencies (
id TEXT NOT NULL PRIMARY KEY,
typename TEXT NOT NULL
)
`
).run();

Is created for that same extract method and used as the target table for a INSERT INTO ... FROM query. Meanwhile we're using the same name of transitive_dependencies as a CTE created from the WITH statement.

transitive_dependencies (id) AS (
VALUES (:rootId) UNION
SELECT direct_dependencies.child_id
FROM transitive_dependencies JOIN direct_dependencies
ON transitive_dependencies.id = direct_dependencies.parent_id
)

Copy link
Contributor

@Beanow Beanow left a comment

Choose a reason for hiding this comment

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

Agree it's better to avoid. Change LGTM

@wchargin
Copy link
Member Author

wchargin commented Feb 2, 2020

Yes, that is correct.

@wchargin wchargin merged commit 0aff308 into master Feb 3, 2020
@wchargin wchargin deleted the wchargin-mirror-no-cte-shadow branch February 3, 2020 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants