aglinxinyuan commented on PR #7703:
URL: https://github.com/apache/texera/pull/7703#issuecomment-5460858500
Thanks — all three addressed, plus one thing the breadcrumb turned up.
**Retitle.** Done, `feat(sql): remove superseded manual update scripts
01-22`. The PR template carries the same rule as AGENTS.md ("removing a
functionality … is a `feat`"), so no argument here.
**Supersede rather than delete.** I've taken the fallback rather than the
primary, and I want to be explicit that it's a choice rather than an oversight.
Your objection is recoverability, and `sql/updates/README.md` is what makes
them recoverable — it records what the 22 were, why none was ever enrolled in
the chain, and the exact `git show` lines to read one or reassemble the set.
What it doesn't do is keep 989 lines of unreachable SQL in the tree, which is
what #7702 asked to remove. If you'd rather the files stay in place with the
README as a header note, say so and I'll flip it — but I don't think a file no
runner can reach earns its place in the tree purely for being browsable.
One wording detail: the README calls `363537e` *a* commit that precedes the
removal, not *the last* one carrying the scripts. `main` still has all 22 until
this merges, so "last" only becomes true afterwards, and I'd rather the
sentence not age wrong. It also lands where the single dangling in-tree pointer
resolves — `DatasetResource.scala:270` cites `sql/updates/18.sql` to explain
`COVER_IMAGE_MAX_PATH_LENGTH = 246`, and a reader following that now arrives at
the README rather than an empty directory.
**The thing I didn't expect.** The breadcrumb is a non-`.sql` file under
`sql/updates/`, and that collides with the DDL-notification workflow. This PR
is labeled `ddl-change` (the labeler adds it for any `**/*.sql` change), and
the workflow's `git diff --diff-filter=A -- 'sql/updates/'` doesn't filter by
extension, so on merge it would have mailed [email protected]:
> please apply sql/updates/README.md to your local Postgres instance and run
`sbt jooqGenerate`
Worth flagging the pre-existing half. As first pushed, this PR would
*already* have mailed that sentence with an empty filename — a deletion-only PR
keeps the label but adds nothing, and the send step has no guard. So the
pathspec now narrows to `sql/updates/*.sql` and the send step gains `if:
steps.get_sql_file.outputs.sql_file != ''`. Six lines in one file, and the only
reason this is no longer a pure deletion. Happy to split it into its own PR if
you'd rather this stay SQL-only — it would just have to land first either way.
Nothing else moved. `bash bin/local-dev/tests/test_local_dev_sh.sh` — 114
passed, the same 3 Windows-only failures as before, and both contract tests
green (`changelog parser: 20 changesets, commented example skipped`,
`changelog: every referenced sql/updates file exists`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
