[bulk] Stop labelling every sql failure "SQL query error" (closes #346) - #347
Merged
Merged
Conversation
`sql` wrapped any exception from spark.sql as "SQL query error: <message>". get_error_message already returns a standalone sentence, so the prefix added nothing to a real query mistake -- "[UNRESOLVED_COLUMN] A column ... Did you mean [sk, pk]?" reads fine on its own -- and actively mislead on failures that were not the query's fault: denial SQL query error: ...is not authorized to perform: dynamodb:Scan... dead executor SQL query error: Task ...rejected from ...ThreadPoolExecutor[Shutting down...] Both became the run's one-line reason, sending a user to re-read SQL that was fine. Dropping the prefix fixes it in one line; the BulkExecutorError wrap stays, so root.py still exits with one clean line and no traceback. Deliberately not doing more. An earlier draft of this change taught the driver to recognise a dead JVM and guess "ran out of memory" from symptoms like ExecutorLostFailure -- but those also mean a spot reclaim or a network partition, so it was a guess, and the client already detects a real memory failure from the executor's own OutOfMemoryError line and Glue's OUT_OF_MEMORY_ERROR category. The driver has no business guessing what the client can see. `load`'s "Could not read the source as '<format>'" is left alone: unlike the SQL prefix it makes a useful, usually-correct claim about the common wrong-format mistake.
hunterhacker
force-pushed
the
explain-dead-jvm-instead-of-blaming-the-verb
branch
from
September 2, 2026 07:14
be184d8 to
3cc8651
Compare
hunterhacker
changed the title
The no-prefix rationale was a six-line essay quoting three example messages; it belongs in the PR, not next to a one-line raise. Cut to why the wrap exists at all (clean one-line exit). Also fixed the passthrough comment above it, which still promised not to wrap things "into an opaque 'SQL query error'" -- a prefix that no longer exists.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #346.
What it does
sqlwrapped any exception asSQL query error: <message>.get_error_messagealready returns a standalone sentence, so the prefix added nothing to a real query mistake and mislabelled failures that were not the query's fault:SQL query error: [UNRESOLVED_COLUMN] A column ... Did you mean [sk, pk]?[UNRESOLVED_COLUMN] A column ... Did you mean [sk, pk]?SQL query error: ...is not authorized......is not authorized...SQL query error: Task ...rejected from ThreadPoolExecutor[...]Task ...rejected from ThreadPoolExecutor[...]One line of behaviour: drop the prefix, keep the
BulkExecutorErrorwrap soroot.pystill exits clean with no traceback.Why this and not more
An earlier version of this branch taught the driver to recognise a dead JVM and phrase it as "ran out of memory". That was a guess --
ExecutorLostFailure/rejected from ThreadPoolExecutoralso mean a spot reclaim or a network partition -- and it duplicated the client, which detects a real memory failure from the executor's ownOutOfMemoryErrorline and Glue'sOUT_OF_MEMORY_ERRORcategory (#348, merged). The driver has no business guessing what the client can see, so that whole layer is gone; this is just the prefix.load'sCould not read the source as '<format>'is left alone -- unlike the SQL prefix it makes a useful, usually-correct claim about the common wrong---formatmistake.Tests
Two existing
test_sqlcases asserted the prefix; both updated -- one now checks the underlying message comes through without it, the other kept its real point (a Spark-side failure is not mislabelled "Invalid 'limit'").Server-side, so re-bootstrap after merging.
make test: 1187 server passing.