Skip to content
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

Fixes structs to handle non-text struct fields. #450

Merged
merged 4 commits into from
Oct 7, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lang/src/org/partiql/lang/errors/ErrorCode.kt
Original file line number Diff line number Diff line change
Expand Up @@ -649,6 +649,11 @@ enum class ErrorCode(private val category: ErrorCategory,
"LIMIT value must be an integer but found ${errorContext.getProperty(Property.ACTUAL_TYPE)}}"
},

EVALUATOR_NON_TEXT_STRUCT_FIELD (
ErrorCategory.EVALUATOR,
LOCATION,
"Struct field should be text."),

EVALUATOR_NEGATIVE_LIMIT(
ErrorCategory.EVALUATOR,
LOCATION,
Expand Down
21 changes: 20 additions & 1 deletion lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt
Original file line number Diff line number Diff line change
Expand Up @@ -868,11 +868,30 @@ internal class EvaluatingCompiler(

val fieldThunks = fields.map {
val (nameExpr, valueExpr) = it
if (nameExpr is Literal && !nameExpr.ionValue.isText) {
// Compile time error. Fail before the StructFieldThunks are evaluated.
err("Found struct field to be of type ${nameExpr.ionValue.type}",
ErrorCode.EVALUATOR_NON_TEXT_STRUCT_FIELD,
errorContextFrom(nameExpr.metas.sourceLocationMeta),
internal = false
)
}
StructFieldThunks(compileExprNode(nameExpr), compileExprNode(valueExpr))
}

return thunkFactory.thunkEnv(metas) { env ->
val seq = fieldThunks.map { it.valueThunk(env).namedValue(it.nameThunk(env)) }.asSequence()
val seq = fieldThunks.map {
val nameValue = it.nameThunk(env)
if (!nameValue.type.isText) {
// Evaluation time error where variable reference might be evaluated to non-text struct field.
err("Found struct field to be of type ${nameValue.type}",
ErrorCode.EVALUATOR_NON_TEXT_STRUCT_FIELD,
errorContextFrom(metas.sourceLocationMeta),
internal = false
)
}
it.valueThunk(env).namedValue(nameValue)
}.asSequence()
createStructExprValue(seq, StructOrdering.ORDERED)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -242,4 +242,37 @@ class EvaluatingCompilerExceptionsTest : EvaluatorTestBase() {
Property.COLUMN_NUMBER to 11L,
Property.BINDING_NAME to "leading")
)

@Test
fun intAsNonTextStructField() =
checkInputThrowingEvaluationException(
"{'apple' : 1, `banana` : 2, 1 : 3}",
ErrorCode.EVALUATOR_NON_TEXT_STRUCT_FIELD,
mapOf(
Property.LINE_NUMBER to 1L,
Property.COLUMN_NUMBER to 29L
)
)

@Test
fun timestampAsNonTextStructField() =
checkInputThrowingEvaluationException(
"{`2007-02-23T12:14Z` : 3}",
ErrorCode.EVALUATOR_NON_TEXT_STRUCT_FIELD,
mapOf(
Property.LINE_NUMBER to 1L,
Property.COLUMN_NUMBER to 2L
)
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May want to also add some tests with NULL and MISSING as the struct field name.

@Test
fun variableReferenceToIntAsNonTextStructField() =
checkInputThrowingEvaluationException(
"SELECT {a : 2} FROM {'a' : 1}",
ErrorCode.EVALUATOR_NON_TEXT_STRUCT_FIELD,
mapOf(
Property.LINE_NUMBER to 1L,
Property.COLUMN_NUMBER to 8L
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some slight inconsistency with the error location. If the non-text struct field name is caught at compile time (as in the previous two tests), the error points to the field name. Whereas, in this case it points to the start of the struct constructor (L8). I think both cases should be consistent with the error pointing to the problematic struct field name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

metas are not available with the ExprValue once it is evaluated.

)
)
}