Update sql validator status #8799
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8799 +/- ##
=========================================
+ Coverage 65.85% 65.9% +0.04%
=========================================
Files 482 482
Lines 24152 24162 +10
Branches 2770 2770
=========================================
+ Hits 15906 15924 +18
+ Misses 8068 8060 -8
Partials 178 178
Continue to review full report at Codecov.
|
| # If we have a message but no error location, return the message and | ||
| # set the location as the beginning. | ||
| return SQLValidationAnnotation( | ||
| message=message, line_number=1, start_column=1, end_column=1 |
There was a problem hiding this comment.
Is this 1-indexed or 0-indexed? (I assume 1 since it's Presto, just checking.)
There was a problem hiding this comment.
1-indexed like you assumed
| f"{validator.name} was unable to check your query.\nPlease " | ||
| "make sure that any services it depends on are available\n" | ||
| f"{validator.name} was unable to check your query.\n" | ||
| "Please recheck your query.\n" |
There was a problem hiding this comment.
Do we know why the validator is unable to check the query? Is this is something the user can fix? Seems to me that there's nothing the user can do, and the validator should be doing the work of checking the query, no?
There was a problem hiding this comment.
It is hard to say. The examples I saw were actually checked by the validator and this PR addresses those cases. So I hope that this will catch issues with our code instead of the validator because you are right that there isn't anything the user can do. I was going to check the logs once this PR was checked in and deployed to see if there were other cases I missed.
mistercrunch
added
🏷️ bot
CATEGORY
Choose one
SUMMARY
The Presto SQL validator does not always return a message and error location. Sometimes we only get a message. If it is an explicit message from the validator, return it and set its error location as the beginning of the string. Otherwise, return it as a general error as a 400 if the message explicitly says it is a 4xx error.
TEST PLAN
Tested manually
REVIEWERS
@betodealmeida @DiggidyDave