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
chore: type ResultSet.tsx #10226
chore: type ResultSet.tsx #10226
Conversation
@@ -387,7 +387,7 @@ export const stoppedQuery = { | |||
startDttm: 1497400851936, | |||
state: 'stopped', | |||
tab: 'Untitled Query 2', | |||
tempTableName: '', | |||
tempTable: '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I aligned on this name everywhere because that's how the backend sends it
// Async queries | ||
let tmpSchema = query.tempSchema; | ||
let tmpTable = query.tempTableName; | ||
// Sync queries, query.results.query contains the source of truth for them. | ||
if (query.results && query.results.query) { | ||
tmpTable = query.results.query.tempTable; | ||
tmpSchema = query.results.query.tempSchema; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic is moved to the reducer, it seems cleaner (although it's a big janky to have the query in 2 different places here)
@@ -235,20 +236,21 @@ export default class ResultSet extends React.PureComponent { | |||
<Alert bsStyle="info"> | |||
{t(object)} [ | |||
<strong> | |||
{tmpSchema}.{tmpTable} | |||
{tempSchema ? `${tempSchema}.` : ''} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When testing on sqllite, there wasn't always a schema, so i cleaned up the cases where schema can be null
37f0968
to
01db9f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks for the quick stamps! I'm going to wait for @bkyryliuk to take a look as well since the CTAS logic was the main functional change here and i want to make sure it looks good to him |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the cleanup, looks good.
Also tested CTAS / CVAS and they are working locally
SUMMARY
As titled. Also includes a couple small refactors/bug fixes that i came across as I was working
TEST PLAN
CI
ADDITIONAL INFORMATION
to: @ktmud @graceguo-supercat @nytai @bkyryliuk