Applied comments in PR 822 (#836)

partiql · Oct 19, 2022 · dbd8c50 · dbd8c50
1 parent c16cc13
commit dbd8c50
Show file tree

Hide file tree

Showing 13 changed files with 119 additions and 73 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -91,6 +91,10 @@ stage in the `PlannerPipeline` and to generate performance metrics for the indiv
 ### Removed
 - README.md badge for travisci
 - **Breaking Change**: removed [ExprValueType.typeNames] as needed by the future work of legacy parser removal and OTS 
+- **Breaking Change**: [PartiqlPhysical.Type.toTypedOpParameter()] now becomes an internal function 
+- **Breaking Change**: [PartiqlAst.Type.toTypedOpParameter()] is removed
+- **Breaking Change**: [PartiqlAstSanityValidator] now becomes an internal class
+- **Breaking Change**: [PartiqlPhysicalSanityValidator] is removed
 
 ### Security
 

diff --git a/lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt b/lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt
@@ -42,6 +42,7 @@ import org.partiql.lang.ast.sourceLocation
 import org.partiql.lang.ast.toAstStatement
 import org.partiql.lang.ast.toPartiQlMetaContainer
 import org.partiql.lang.domains.PartiqlAst
+import org.partiql.lang.domains.PartiqlPhysical
 import org.partiql.lang.domains.staticType
 import org.partiql.lang.domains.toBindingCase
 import org.partiql.lang.errors.ErrorCode
@@ -1221,7 +1222,7 @@ internal class EvaluatingCompiler(
 
     private fun compileIs(expr: PartiqlAst.Expr.IsType, metas: MetaContainer): ThunkEnv {
         val expThunk = compileAstExpr(expr.value)
-        val typedOpParameter = expr.type.toTypedOpParameter(customTypedOpParameters)
+        val typedOpParameter = expr.type.toTypedOpParameter()
         if (typedOpParameter.staticType is AnyType) {
             return thunkFactory.thunkEnv(metas) { valueFactory.newBoolean(true) }
         }
@@ -1265,7 +1266,7 @@ internal class EvaluatingCompiler(
 
     private fun compileCastHelper(value: PartiqlAst.Expr, asType: PartiqlAst.Type, metas: MetaContainer): ThunkEnv {
         val expThunk = compileAstExpr(value)
-        val typedOpParameter = asType.toTypedOpParameter(customTypedOpParameters)
+        val typedOpParameter = asType.toTypedOpParameter()
         if (typedOpParameter.staticType is AnyType) {
             return expThunk
         }
@@ -1361,7 +1362,7 @@ internal class EvaluatingCompiler(
         thunkFactory.thunkEnv(metas, compileCastHelper(expr.value, expr.asType, metas))
 
     private fun compileCanCast(expr: PartiqlAst.Expr.CanCast, metas: MetaContainer): ThunkEnv {
-        val typedOpParameter = expr.asType.toTypedOpParameter(customTypedOpParameters)
+        val typedOpParameter = expr.asType.toTypedOpParameter()
         if (typedOpParameter.staticType is AnyType) {
             return thunkFactory.thunkEnv(metas) { valueFactory.newBoolean(true) }
         }
@@ -1396,7 +1397,7 @@ internal class EvaluatingCompiler(
     }
 
     private fun compileCanLosslessCast(expr: PartiqlAst.Expr.CanLosslessCast, metas: MetaContainer): ThunkEnv {
-        val typedOpParameter = expr.asType.toTypedOpParameter(customTypedOpParameters)
+        val typedOpParameter = expr.asType.toTypedOpParameter()
         if (typedOpParameter.staticType is AnyType) {
             return thunkFactory.thunkEnv(metas) { valueFactory.newBoolean(true) }
         }
@@ -3014,6 +3015,22 @@ internal class EvaluatingCompiler(
             },
             ordering
         )
+
+    /** Helper to convert [PartiqlAst.Type] in AST to a [TypedOpParameter]. */
+    private fun PartiqlAst.Type.toTypedOpParameter(): TypedOpParameter {
+        // hack: to avoid duplicating the function `PartiqlAst.Type.toTypedOpParameter`, we have to convert this
+        // PartiqlAst.Type to PartiqlPhysical.Type. The easiest way to do that without using a visitor transform
+        // (which is overkill and comes with some downsides for something this simple), is to transform to and from
+        // s-expressions again.  This will work without difficulty as long as PartiqlAst.Type remains unchanged in all
+        // permuted domains between PartiqlAst and PartiqlPhysical.
+
+        // This is really just a temporary measure, however, which must exist for as long as the type inferencer works only
+        // on PartiqlAst.  When it has been migrated to use PartiqlPhysical instead, there should no longer be a reason
+        // to keep this function around.
+        val sexp = this.toIonElement()
+        val physicalType = PartiqlPhysical.transform(sexp) as PartiqlPhysical.Type
+        return physicalType.toTypedOpParameter(customTypedOpParameters)
+    }
 }
 
 /**

diff --git a/lang/src/org/partiql/lang/eval/physical/PhysicalPlanCompilerImpl.kt b/lang/src/org/partiql/lang/eval/physical/PhysicalPlanCompilerImpl.kt
@@ -85,7 +85,6 @@ import org.partiql.lang.eval.stringValue
 import org.partiql.lang.eval.syntheticColumnName
 import org.partiql.lang.eval.time.Time
 import org.partiql.lang.eval.unnamedValue
-import org.partiql.lang.eval.visitors.PartiqlPhysicalSanityValidator
 import org.partiql.lang.planner.EvaluatorOptions
 import org.partiql.lang.types.AnyOfType
 import org.partiql.lang.types.AnyType
@@ -174,8 +173,6 @@ internal class PhysicalPlanCompilerImpl(
      * hope that long-running compilations may be aborted by the caller.
      */
     fun compile(plan: PartiqlPhysical.Plan): Expression {
-        PartiqlPhysicalSanityValidator(evaluatorOptions).walkPlan(plan)
-
         val thunk = compileAstStatement(plan.stmt)
 
         return object : Expression {

diff --git a/lang/src/org/partiql/lang/eval/visitors/PartiqlAstSanityValidator.kt b/lang/src/org/partiql/lang/eval/visitors/PartiqlAstSanityValidator.kt
@@ -45,11 +45,11 @@ import org.partiql.lang.types.TYPE_ALIAS_TO_SCALAR_TYPE
  * - A visitor transform pass (internal or external)
  *
  */
-class PartiqlAstSanityValidator : PartiqlAst.Visitor() {
+internal class PartiqlAstSanityValidator : PartiqlAst.Visitor() {
 
     private var compileOptions = CompileOptions.standard()
 
-    fun validate(statement: PartiqlAst.Statement, compileOptions: CompileOptions = CompileOptions.standard()) {
+    internal fun validate(statement: PartiqlAst.Statement, compileOptions: CompileOptions = CompileOptions.standard()) {
         this.compileOptions = compileOptions
         this.walkStatement(statement)
     }

diff --git a/lang/src/org/partiql/lang/eval/visitors/StaticTypeInferenceVisitorTransform.kt b/lang/src/org/partiql/lang/eval/visitors/StaticTypeInferenceVisitorTransform.kt
@@ -23,6 +23,7 @@ import org.partiql.lang.ast.passes.inference.isText
 import org.partiql.lang.ast.passes.inference.isUnknown
 import org.partiql.lang.ast.passes.inference.toStaticType
 import org.partiql.lang.domains.PartiqlAst
+import org.partiql.lang.domains.PartiqlPhysical
 import org.partiql.lang.domains.staticType
 import org.partiql.lang.domains.toBindingCase
 import org.partiql.lang.errors.Problem
@@ -1199,7 +1200,7 @@ internal class StaticTypeInferenceVisitorTransform(
         override fun transformExprCast(node: PartiqlAst.Expr.Cast): PartiqlAst.Expr {
             val typed = super.transformExprCast(node) as PartiqlAst.Expr.Cast
             val sourceType = typed.value.getStaticType()
-            val targetType = typed.asType.toTypedOpParameter(customTypedOpParameters)
+            val targetType = typed.asType.toTypedOpParameter()
             val castOutputType = sourceType.cast(targetType.staticType, plugin).let {
                 if (targetType.validationThunk == null) {
                     // There is no additional validation for this parameter, return this type as-is
@@ -1621,4 +1622,20 @@ internal class StaticTypeInferenceVisitorTransform(
             }
         }
     }
+
+    /** Helper to convert [PartiqlAst.Type] in AST to a [TypedOpParameter]. */
+    private fun PartiqlAst.Type.toTypedOpParameter(): TypedOpParameter {
+        // hack: to avoid duplicating the function `PartiqlAst.Type.toTypedOpParameter`, we have to convert this
+        // PartiqlAst.Type to PartiqlPhysical.Type. The easiest way to do that without using a visitor transform
+        // (which is overkill and comes with some downsides for something this simple), is to transform to and from
+        // s-expressions again.  This will work without difficulty as long as PartiqlAst.Type remains unchanged in all
+        // permuted domains between PartiqlAst and PartiqlPhysical.
+
+        // This is really just a temporary measure, however, which must exist for as long as the type inferencer works only
+        // on PartiqlAst.  When it has been migrated to use PartiqlPhysical instead, there should no longer be a reason
+        // to keep this function around.
+        val sexp = this.toIonElement()
+        val physicalType = PartiqlPhysical.transform(sexp) as PartiqlPhysical.Type
+        return physicalType.toTypedOpParameter(customTypedOpParameters)
+    }
 }
diff --git a/lang/src/org/partiql/lang/planner/PartiQLPlanner.kt b/lang/src/org/partiql/lang/planner/PartiQLPlanner.kt
@@ -17,6 +17,7 @@ package org.partiql.lang.planner
 import org.partiql.lang.domains.PartiqlAst
 import org.partiql.lang.domains.PartiqlPhysical
 import org.partiql.lang.errors.Problem
+import org.partiql.lang.eval.TypedOpBehavior
 
 /**
  * [PartiQLPlanner] is responsible for transforming a [PartiqlAst.Statement] representation of a query into an
@@ -57,5 +58,8 @@ interface PartiQLPlanner {
     /**
      * Options which control [PartiQLPlanner] behavior.
      */
-    class Options(val allowedUndefinedVariables: Boolean = false)
+    class Options(
+        val allowedUndefinedVariables: Boolean = false,
+        val typedOpBehavior: TypedOpBehavior = TypedOpBehavior.LEGACY
+    )
 }
diff --git a/lang/src/org/partiql/lang/planner/PartiQLPlannerDefault.kt b/lang/src/org/partiql/lang/planner/PartiQLPlannerDefault.kt
@@ -27,6 +27,8 @@ import org.partiql.lang.planner.transforms.AstToLogicalVisitorTransform
 import org.partiql.lang.planner.transforms.LogicalResolvedToDefaultPhysicalVisitorTransform
 import org.partiql.lang.planner.transforms.LogicalToLogicalResolvedVisitorTransform
 import org.partiql.lang.planner.transforms.allocateVariableIds
+import org.partiql.lang.planner.validators.PartiqlLogicalResolvedValidator
+import org.partiql.lang.planner.validators.PartiqlLogicalValidator
 import org.partiql.pig.runtime.asPrimitive
 
 internal class PartiQLPlannerDefault(
@@ -55,6 +57,9 @@ internal class PartiQLPlannerDefault(
         if (problemHandler.hasErrors) {
             return PartiQLPlanner.Result.Error(problemHandler.problems)
         }
+        // Validate logical plan
+        // TODO: if it is an invalid logical plan, do we want to add it to [problemHandler]?
+        PartiqlLogicalValidator(options.typedOpBehavior).walkPlan(logicalPlan)
 
         // Step 3. Replace variable references
         val resolvedLogicalPlan = callback.doEvent("logical_to_logical_resolved", logicalPlan) {
@@ -63,6 +68,7 @@ internal class PartiQLPlannerDefault(
         if (problemHandler.hasErrors) {
             return PartiQLPlanner.Result.Error(problemHandler.problems)
         }
+        PartiqlLogicalResolvedValidator().walkPlan(resolvedLogicalPlan)
 
         // Step 4. LogicalPlan -> PhysicalPlan
         val physicalPlan = callback.doEvent("logical_resolved_to_physical", resolvedLogicalPlan) {

diff --git a/lang/src/org/partiql/lang/planner/PlannerPipeline.kt b/lang/src/org/partiql/lang/planner/PlannerPipeline.kt
@@ -41,6 +41,8 @@ import org.partiql.lang.planner.transforms.normalize
 import org.partiql.lang.planner.transforms.toDefaultPhysicalPlan
 import org.partiql.lang.planner.transforms.toLogicalPlan
 import org.partiql.lang.planner.transforms.toResolvedPlan
+import org.partiql.lang.planner.validators.PartiqlLogicalResolvedValidator
+import org.partiql.lang.planner.validators.PartiqlLogicalValidator
 import org.partiql.lang.syntax.Parser
 import org.partiql.lang.syntax.PartiQLParserBuilder
 import org.partiql.lang.syntax.SyntaxException
@@ -493,6 +495,8 @@ internal class PlannerPipelineImpl(
             return PlannerPassResult.Error(problemHandler.problems)
         }
 
+        PartiqlLogicalValidator(evaluatorOptions.typedOpBehavior).walkPlan(logicalPlan)
+
         // logical plan -> resolved logical plan
         val resolvedLogicalPlan = plannerEventCallback.doEvent("logical_to_logical_resolved", logicalPlan) {
             logicalPlan.toResolvedPlan(problemHandler, globalVariableResolver, allowUndefinedVariables)
@@ -503,6 +507,8 @@ internal class PlannerPipelineImpl(
             return PlannerPassResult.Error(problemHandler.problems)
         }
 
+        PartiqlLogicalResolvedValidator().walkPlan(resolvedLogicalPlan)
+
         // Possible future passes:
         // - type checking and inferencing?
         // - constant folding

diff --git a/lang/src/org/partiql/lang/planner/validators/PartiqlLogicalResolvedValidator.kt b/lang/src/org/partiql/lang/planner/validators/PartiqlLogicalResolvedValidator.kt
@@ -0,0 +1,35 @@
+package org.partiql.lang.planner.validators
+
+import org.partiql.lang.domains.PartiqlLogicalResolved
+import org.partiql.lang.errors.ErrorCode
+import org.partiql.lang.eval.EvaluationException
+import org.partiql.lang.util.propertyValueMapOf
+
+/**
+ * Provides rules for basic AST sanity checks that should be performed before any attempt at further AST processing.
+ * This is provided as a distinct [PartiqlLogicalResolved.Visitor] so that all other visitors may assume that the AST at least
+ * passed the checking performed here.
+ *
+ * Any exception thrown by this class should always be considered an indication of a bug in one of the following places:
+ * - [org.partiql.lang.planner.transforms.LogicalToLogicalResolvedVisitorTransform]
+ */
+class PartiqlLogicalResolvedValidator : PartiqlLogicalResolved.Visitor() {
+    /**
+     * Quick validation step to make sure the indexes of any variables make sense.
+     * It is unlikely that this check will ever fail, but if it does, it likely means there's a bug in
+     * [org.partiql.lang.planner.transforms.VariableIdAllocator] or that the plan was malformed by other means.
+     */
+    override fun visitPlan(node: PartiqlLogicalResolved.Plan) {
+        node.locals.forEachIndexed { idx, it ->
+            if (it.registerIndex.value != idx.toLong()) {
+                throw EvaluationException(
+                    message = "Variable index must match ordinal position of variable",
+                    errorCode = ErrorCode.INTERNAL_ERROR,
+                    errorContext = propertyValueMapOf(),
+                    internal = true
+                )
+            }
+        }
+        super.visitPlan(node)
+    }
+}
diff --git a/...isitors/PartiqlPhysicalSanityValidator.kt → ...ner/validators/PartiqlLogicalValidator.kt b/...isitors/PartiqlPhysicalSanityValidator.kt → ...ner/validators/PartiqlLogicalValidator.kt
@@ -1,12 +1,12 @@
-package org.partiql.lang.eval.visitors
+package org.partiql.lang.planner.validators
 
 import com.amazon.ionelement.api.IntElement
 import com.amazon.ionelement.api.IntElementSize
 import com.amazon.ionelement.api.MetaContainer
 import com.amazon.ionelement.api.TextElement
 import org.partiql.lang.ast.IsCountStarMeta
 import org.partiql.lang.ast.passes.SemanticException
-import org.partiql.lang.domains.PartiqlPhysical
+import org.partiql.lang.domains.PartiqlLogical
 import org.partiql.lang.domains.addSourceLocation
 import org.partiql.lang.errors.ErrorCode
 import org.partiql.lang.errors.Property
@@ -15,40 +15,19 @@ import org.partiql.lang.eval.EvaluationException
 import org.partiql.lang.eval.TypedOpBehavior
 import org.partiql.lang.eval.err
 import org.partiql.lang.eval.errorContextFrom
-import org.partiql.lang.planner.EvaluatorOptions
 import org.partiql.lang.types.BuiltInScalarType
 import org.partiql.lang.types.TYPE_ALIAS_TO_SCALAR_TYPE
-import org.partiql.lang.util.propertyValueMapOf
 
 /**
- * Provides rules for basic AST sanity checks that should be performed before any attempt at further physical
- * plan processing. This is provided as a distinct [PartiqlPhysical.Visitor] so that the planner and evaluator may
- * assume that the physical plan has passed the checks performed here.
+ * Provides rules for basic AST sanity checks that should be performed before any attempt at further AST processing.
+ * This is provided as a distinct [PartiqlLogical.Visitor] so that all other visitors may assume that the AST at least
+ * passed the checking performed here.
  *
- * Any exception thrown by this class should always be considered an indication of a bug.
+ * Any exception thrown by this class should always be considered an indication of a bug in one of the following places:
+ * - [org.partiql.lang.planner.transforms.AstToLogicalVisitorTransform]
  */
-class PartiqlPhysicalSanityValidator(private val evaluatorOptions: EvaluatorOptions) : PartiqlPhysical.Visitor() {
-
-    /**
-     * Quick validation step to make sure the indexes of any variables make sense.
-     * It is unlikely that this check will ever fail, but if it does, it likely means there's a bug in
-     * [org.partiql.lang.planner.transforms.VariableIdAllocator] or that the plan was malformed by other means.
-     */
-    override fun visitPlan(node: PartiqlPhysical.Plan) {
-        node.locals.forEachIndexed { idx, it ->
-            if (it.registerIndex.value != idx.toLong()) {
-                throw EvaluationException(
-                    message = "Variable index must match ordinal position of variable",
-                    errorCode = ErrorCode.INTERNAL_ERROR,
-                    errorContext = propertyValueMapOf(),
-                    internal = true
-                )
-            }
-        }
-        super.visitPlan(node)
-    }
-
-    override fun visitExprLit(node: PartiqlPhysical.Expr.Lit) {
+class PartiqlLogicalValidator(private val typedOpBehavior: TypedOpBehavior) : PartiqlLogical.Visitor() {
+    override fun visitExprLit(node: PartiqlLogical.Expr.Lit) {
         val ionValue = node.value
         val metas = node.metas
         if (node.value is IntElement && ionValue.integerSize == IntElementSize.BIG_INTEGER) {
@@ -62,7 +41,7 @@ class PartiqlPhysicalSanityValidator(private val evaluatorOptions: EvaluatorOpti
     }
 
     private fun validateDecimalOrNumericType(precision: Long?, scale: Long?, metas: MetaContainer) {
-        if (scale != null && precision != null && evaluatorOptions.typedOpBehavior == TypedOpBehavior.HONOR_PARAMETERS) {
+        if (scale != null && precision != null && typedOpBehavior == TypedOpBehavior.HONOR_PARAMETERS) {
             if (scale !in 0..precision) {
                 err(
                     "Scale $scale should be between 0 and precision $precision",
@@ -74,7 +53,7 @@ class PartiqlPhysicalSanityValidator(private val evaluatorOptions: EvaluatorOpti
         }
     }
 
-    override fun visitTypeScalarType(node: PartiqlPhysical.Type.ScalarType) {
+    override fun visitTypeScalarType(node: PartiqlLogical.Type.ScalarType) {
         super.visitTypeScalarType(node)
 
         val scalarType = TYPE_ALIAS_TO_SCALAR_TYPE[node.alias.text]
@@ -83,10 +62,10 @@ class PartiqlPhysicalSanityValidator(private val evaluatorOptions: EvaluatorOpti
         }
     }
 
-    override fun visitExprCallAgg(node: PartiqlPhysical.Expr.CallAgg) {
+    override fun visitExprCallAgg(node: PartiqlLogical.Expr.CallAgg) {
         val setQuantifier = node.setq
         val metas = node.metas
-        if (setQuantifier is PartiqlPhysical.SetQuantifier.Distinct && metas.containsKey(IsCountStarMeta.TAG)) {
+        if (setQuantifier is PartiqlLogical.SetQuantifier.Distinct && metas.containsKey(IsCountStarMeta.TAG)) {
             err(
                 "COUNT(DISTINCT *) is not supported",
                 ErrorCode.EVALUATOR_COUNT_DISTINCT_STAR,
@@ -96,15 +75,15 @@ class PartiqlPhysicalSanityValidator(private val evaluatorOptions: EvaluatorOpti
         }
     }
 
-    override fun visitExprStruct(node: PartiqlPhysical.Expr.Struct) {
+    override fun visitExprStruct(node: PartiqlLogical.Expr.Struct) {
         node.parts.forEach { part ->
             when (part) {
-                is PartiqlPhysical.StructPart.StructField -> {
-                    if (part.fieldName is PartiqlPhysical.Expr.Missing ||
-                        (part.fieldName is PartiqlPhysical.Expr.Lit && part.fieldName.value !is TextElement)
+                is PartiqlLogical.StructPart.StructField -> {
+                    if (part.fieldName is PartiqlLogical.Expr.Missing ||
+                        (part.fieldName is PartiqlLogical.Expr.Lit && part.fieldName.value !is TextElement)
                     ) {
                         val type = when (part.fieldName) {
-                            is PartiqlPhysical.Expr.Lit -> part.fieldName.value.type.toString()
+                            is PartiqlLogical.Expr.Lit -> part.fieldName.value.type.toString()
                             else -> "MISSING"
                         }
                         throw SemanticException(
@@ -116,7 +95,7 @@ class PartiqlPhysicalSanityValidator(private val evaluatorOptions: EvaluatorOpti
                         )
                     }
                 }
-                is PartiqlPhysical.StructPart.StructFields -> { /* intentionally empty */ }
+                is PartiqlLogical.StructPart.StructFields -> { /* intentionally empty */ }
             }
         }
     }