Uh oh!
There was an error while loading. Please reload this page.
Add support for TABLESAMPLE - #1580
Conversation
Uh oh!
There was an error while loading. Please reload this page.
| sample: Option<Box<TableSample>>, | ||
| /// Position of the table sample modifier in the table factor. Default is after the table alias | ||
| /// e.g. `SELECT * FROM tbl t TABLESAMPLE (10 ROWS)`. See `Dialect::supports_table_sample_before_alias`. | ||
| sample_before_alias: bool, |
There was a problem hiding this comment.
Would it make sense to use an enum here? e.g
enumTableSampleKind{BeforeTableAlias(Box<TableSample>)// ...}
sample:Option<TableSampleKind>thinking that avoids the surplus flag on the table factor and would lend itself to be extensible if required later on
| self.expect_token(&Token::RParen)?; | ||
| TableSample::Bucket(TableSampleBucket { bucket, total, on }) | ||
| } else { | ||
| let value = match self.try_parse(|p| p.parse_number_value()) { |
There was a problem hiding this comment.
Should the try_parse rather be maybe_parse (we seem to be ignoring the returned error otherwise)?
| let value = match self.maybe_parse(|p| p.parse_number_value()) { | ||
| Ok(Some(num)) => num, | ||
| _ => { |
There was a problem hiding this comment.
| let value = matchself.maybe_parse(|p| p.parse_number_value()){ | |
| Ok(Some(num)) => num, | |
| _ => { | |
| let value = matchself.maybe_parse(|p| p.parse_number_value())?{ | |
| Some(num) => num, | |
| None => { |
I think this is usually the recursion limit or similar fatal error we can propagate
| if self.peek_token().token == Token::RParen | ||
| && !self.dialect.supports_implicit_table_sample_method() |
There was a problem hiding this comment.
maybe this could be simplified as if self.dialect.supports_implicit_table_sample_method() && self.consume_token(Token::RParen) it would also let us skip the expect_token that follows as well?
| repeatable: seed, | ||
| }) | ||
| // Try to parse without an explicit table sample method keyword | ||
| } else if self.peek_token().token == Token::LParen { |
There was a problem hiding this comment.
| }elseifself.peek_token().token == Token::LParen{ | |
| }elseifself.consume_token(Token::LParen){ |
| "SELECT * FROM testtable SAMPLE (10)", | ||
| "SELECT * FROM testtable TABLESAMPLE BERNOULLI (10)", |
There was a problem hiding this comment.
for the scenarios that currently rely on one_statement_parse_to, could we represent them faithfully when displaying? e.g this and the ROW vs BERNOULLI variants etc
There was a problem hiding this comment.
Not sure if I understand the question, but if you mean whether the variants I added as interchangeable are really interchangeable in the dialect, as far as I understand yes. If not, please elaborate?
There was a problem hiding this comment.
Ah yeah so I was rather thinking that ideally we preserve the syntax roundtrip, even though they'r interchangeable in some dialects
snowflake_and_generic().verified_stmt("SELECT * FROM testtable TABLESAMPLE ROW (20.3)");snowflake_and_generic().verified_stmt("SELECT * FROM testtable SAMPLE BLOCK (3) SEED (82)");yoavcloud
commented
Dec 9, 2024
@iffyio please see latest commit |
iffyio
left a comment
There was a problem hiding this comment.
Thanks @yoavcloud! Left a comment to distinguish sample vs tablesample. Otherwise this looks good to me!
| snowflake_and_generic().one_statement_parses_to( | ||
| "SELECT * FROM testtable SAMPLE (10)", | ||
| "SELECT * FROM testtable TABLESAMPLE (10)", | ||
| ); |
There was a problem hiding this comment.
Oh could we do the same here as well? clickhouse for example doesn't have has SAMPLE but not TABLESAMPLE so that the roundtrip stays correct
There was a problem hiding this comment.
Added support for clickhouse as well, with a different approach to how to model the it in the AST
iffyio
left a comment
There was a problem hiding this comment.
LGTM! Thanks @yoavcloud
cc @alamb
alamb
commented
Dec 15, 2024
Thanks @yoavcloud and @iffyio |
This PR adds support for the
TABLESAMPLEoption in the following dialects:Collateral work includes expanding the use of a constructor function to create
Tablestructs in unit tests to avoid modifying many files when adding default options to theTablestruct.