-
Notifications
You must be signed in to change notification settings - Fork 14
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
#34 Support Update Statement #97
#34 Support Update Statement #97
Conversation
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.
Few changes, and corner cases to test.
@@ -68,4 +69,18 @@ class UpdateStatementParserTest extends FlatSpec with Matchers { | |||
simpleRelation2.term.asInstanceOf[Constant].raw shouldBe "click" | |||
|
|||
} | |||
|
|||
it should "parse update statement with list literal assignment" in { |
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.
Please add more test cases for the parser. Also, test for BindMarkers in different places of the statement.
@@ -32,6 +32,12 @@ object Messages { | |||
// Validations | |||
case class SelectedDistinctNonStaticColumn(c: Identifier) extends Message(s"SELECT DISTINCT queries must only request partition key columns and/or static columns (not $c)") | |||
|
|||
// TODO: Format Term |
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.
Please move those new classes to line 26.
schema.getTable(s.tableName).flatMap { table => | ||
V.merge(Seq( | ||
extractVariablesFomSet(table, s.set), | ||
extractVariablesFromUpdateParam(table, s.using), |
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.
Wrong order. Variables in USING
should appear before variables in SET
.
Please first write test cases to expose the problem.
|
||
private def extractVariablesFromEitherListLiteralOrBindMarker(table: Table, columnName: Identifier, literalOrBindMarker: Either[ListLiteral, BindMarker]): Result[Seq[DataType]] = { | ||
literalOrBindMarker match { | ||
case Left(listLiteral) => extractVariablesFromListLiteralAssignment(table, columnName, listLiteral) |
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.
Please add a todo to validate that column is actually a list.
private def extractVariablesFromTermAssigment(table: Table, columnName: Identifier, updateOperator: Update.UpdateOperator, term: Term): Result[Seq[DataType]] = | ||
table.getColumn(columnName).map(_.dataType).flatMap { | ||
case DataType.List(t) => extractVariablesFromTerm(term, DataType.List(t)) | ||
case DataType.Map(k, t) => extractVariablesFromTerm(term, DataType.Map(k, t)) |
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.
What about DataType.Set
?
@tabdulradi done, check the new commits please. |
Please write a test case for the variable order bug first. |
No description provided.