This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7249-7daf8d78415d5f40fc017678738ce57b137065c5 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 71faf440de7936d07451d214dbd5777e6906b949 Author: Kary Zheng <[email protected]> AuthorDate: Wed Aug 12 21:46:55 2026 +0000 feat(visualization): retarget attributeTypeRules at the properties they name (#7249) ### What changes were proposed in this PR? Four `attributeTypeRules` name keys that match no property, so the property editor's `findAttributeType` returns `undefined` and `checkConstraint` returns without checking anything. `LineConfig` used the Scala field names rather than the `@JsonProperty` names, and `ScatterMatrixChartOpDesc` named `value` where the property is `Selected Attributes`; both are retargeted at the property they meant. `Scatter3dChartOpDesc` and `FunnelPlotOpDesc` constrained a `title` that neither operator declares, and did so with the bare string `"string"` rather than an object, so even under a correct key `constraint.enum` / `const` / `allOf` would all be undefined and the check would still no-op; those two rules are removed rather than invented anew. ### Why are the changes needed? A line chart's x and y axes accept string columns today despite declaring `["integer", "long", "double"]`, and the same holds for the scatter matrix's dimensions. `BandConfig` extends `LineConfig` and inherited the same dead rule. Nothing reports a key that names no property, so the rules read as enforced while enforcing nothing. ### Any related issues, documentation, discussions? Closes #7210 ### How was this PR tested? `WorkflowOperator/compile`, `WorkflowOperator/scalafmtCheckAll`, and the four operators' existing descriptor specs (24 tests, all passing). The new `AttributeTypeRuleTargetSpec` guards the class of mistake repo-wide rather than just the four sites fixed here: it walks every registered operator's generated schema and fails if an `attributeTypeRules` key names no declared property, or if a rule is not stated as an object. ### Does this PR introduce any user-facing change? Yes. Selecting a non-numeric column for a line chart's axes or the scatter matrix's Selected Attributes now shows the type warning the rule always intended. Nothing changes for a numeric column, and removing the two `title` rules changes nothing at all, since they never applied. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5) --------- Co-authored-by: Claude Opus 5 (1M context) <[email protected]> Co-authored-by: Xuan Gu <[email protected]> --- .../ScatterMatrixChartOpDesc.scala | 5 +- .../funnelPlot/FunnelPlotOpDesc.scala | 9 +- .../visualization/lineChart/LineConfig.scala | 8 +- .../scatter3DChart/Scatter3dChartOpDesc.scala | 9 +- .../metadata/AttributeTypeRuleTargetSpec.scala | 111 +++++++++++++++++++++ 5 files changed, 122 insertions(+), 20 deletions(-) diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/ScatterMatrixChart/ScatterMatrixChartOpDesc.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/ScatterMatrixChart/ScatterMatrixChartOpDesc.scala index e6397d1877..f30b5cee90 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/ScatterMatrixChart/ScatterMatrixChartOpDesc.scala +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/ScatterMatrixChart/ScatterMatrixChartOpDesc.scala @@ -34,10 +34,13 @@ import org.apache.texera.amber.operator.metadata.{OperatorGroupConstants, Operat import org.apache.texera.amber.pybuilder.PythonTemplateBuilder import javax.validation.constraints.{NotEmpty, NotNull} +// Type constraint: every axis of the matrix is plotted numerically. The key is the +// PROPERTY name -- the `@JsonProperty` value, which is what the property editor looks +// up -- and this operator's is "Selected Attributes"; keyed "value" it matched nothing. @JsonSchemaInject(json = """ { "attributeTypeRules": { - "value": { + "Selected Attributes": { "enum": ["integer", "long", "double"] } } diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/funnelPlot/FunnelPlotOpDesc.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/funnelPlot/FunnelPlotOpDesc.scala index cd9f38619d..eb83ed5000 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/funnelPlot/FunnelPlotOpDesc.scala +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/funnelPlot/FunnelPlotOpDesc.scala @@ -20,7 +20,7 @@ package org.apache.texera.amber.operator.visualization.funnelPlot import com.fasterxml.jackson.annotation.{JsonProperty, JsonPropertyDescription} -import com.kjetland.jackson.jsonSchema.annotations.{JsonSchemaInject, JsonSchemaTitle} +import com.kjetland.jackson.jsonSchema.annotations.JsonSchemaTitle import org.apache.texera.amber.core.tuple.{AttributeType, Schema} import org.apache.texera.amber.pybuilder.PythonTemplateBuilder.PythonTemplateBuilderStringContext import org.apache.texera.amber.pybuilder.PyStringTypes.EncodableString @@ -31,13 +31,6 @@ import org.apache.texera.amber.operator.metadata.{OperatorGroupConstants, Operat import org.apache.texera.amber.pybuilder.PythonTemplateBuilder import javax.validation.constraints.NotNull -@JsonSchemaInject(json = """ -{ - "attributeTypeRules": { - "title": "string" - } -} -""") class FunnelPlotOpDesc extends PythonOperatorDescriptor { @JsonProperty(required = true) diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/lineChart/LineConfig.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/lineChart/LineConfig.scala index 09f682e12e..82b9210c98 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/lineChart/LineConfig.scala +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/lineChart/LineConfig.scala @@ -26,14 +26,16 @@ import org.apache.texera.amber.operator.metadata.annotations.AutofillAttributeNa import javax.validation.constraints.NotNull -//type constraint: value can only be numeric +// Type constraint: both axes can only be numeric. The keys are the PROPERTY names -- +// the `@JsonProperty` values, which is what the property editor looks up -- not the +// Scala field names; keyed by the latter the rule matched nothing. @JsonSchemaInject(json = """ { "attributeTypeRules": { - "yValue": { + "y": { "enum": ["integer", "long", "double"] }, - "xValue": { + "x": { "enum": ["integer", "long", "double"] } } diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/scatter3DChart/Scatter3dChartOpDesc.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/scatter3DChart/Scatter3dChartOpDesc.scala index e9976f56e3..c86d9e3cc6 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/scatter3DChart/Scatter3dChartOpDesc.scala +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/scatter3DChart/Scatter3dChartOpDesc.scala @@ -20,7 +20,7 @@ package org.apache.texera.amber.operator.visualization.scatter3DChart import com.fasterxml.jackson.annotation.{JsonProperty, JsonPropertyDescription} -import com.kjetland.jackson.jsonSchema.annotations.{JsonSchemaInject, JsonSchemaTitle} +import com.kjetland.jackson.jsonSchema.annotations.JsonSchemaTitle import org.apache.texera.amber.core.tuple.{AttributeType, Schema} import org.apache.texera.amber.pybuilder.PythonTemplateBuilder.PythonTemplateBuilderStringContext import org.apache.texera.amber.pybuilder.PyStringTypes.EncodableString @@ -31,13 +31,6 @@ import org.apache.texera.amber.operator.metadata.{OperatorGroupConstants, Operat import org.apache.texera.amber.pybuilder.PythonTemplateBuilder import javax.validation.constraints.NotNull -@JsonSchemaInject(json = """ -{ - "attributeTypeRules": { - "title": "string" - } -} -""") class Scatter3dChartOpDesc extends PythonOperatorDescriptor { @JsonProperty(value = "x", required = true) @JsonSchemaTitle("X Column") diff --git a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/metadata/AttributeTypeRuleTargetSpec.scala b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/metadata/AttributeTypeRuleTargetSpec.scala new file mode 100644 index 0000000000..0bd4b2b8f1 --- /dev/null +++ b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/metadata/AttributeTypeRuleTargetSpec.scala @@ -0,0 +1,111 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.operator.metadata + +import com.fasterxml.jackson.databind.JsonNode +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import scala.jdk.CollectionConverters._ + +/** + * Guard for `attributeTypeRules`, over every registered operator. + * + * The property editor looks a rule's key up among the enclosing object's properties, + * by the `@JsonProperty` name. A key that names no property, or a rule that is not an + * object, makes the editor's constraint check return without checking anything -- the + * rule reads as enforced while enforcing nothing, and no existing test notices, + * because the operator still compiles and still runs. + * + * Sweeping every operator rather than the ones a fix happened to touch is deliberate: + * the mistake is invisible at the declaration site, so it is as easy to make in the + * next operator as it was in these. + */ +class AttributeTypeRuleTargetSpec extends AnyFlatSpec with Matchers { + + private val RulesKeyword = "attributeTypeRules" + + /** One declared rule, and the properties it could legally have named. */ + private case class Rule( + schemaPath: String, + key: String, + value: JsonNode, + declaredProperties: Set[String] + ) { + override def toString: String = + s"$schemaPath.$RulesKeyword.$key " + + s"(declared properties at $schemaPath: ${declaredProperties.toSeq.sorted.mkString(", ")})" + } + + /** + * Rules are injected per class, so they appear both at the top level and, for a + * nested config class, under its own definition -- each alongside the properties it + * constrains. Collect them wherever they sit. + */ + private def rulesIn(node: JsonNode, schemaPath: String): Seq[Rule] = { + if (!node.isObject) return Seq.empty + + val here = if (node.has(RulesKeyword)) { + val rules = node.path(RulesKeyword) + val properties = node.path("properties").fieldNames().asScala.toSet + rules.fieldNames().asScala.toSeq.map { key => + Rule(schemaPath, key, rules.path(key), properties) + } + } else Seq.empty + + here ++ node + .fields() + .asScala + .toSeq + .flatMap(child => rulesIn(child.getValue, s"$schemaPath.${child.getKey}")) + } + + private val rulesByOperator: Seq[(String, Seq[Rule])] = + OperatorMetadataGenerator.operatorTypeMap.keys.toSeq + .map(opClass => + opClass.getSimpleName -> rulesIn( + OperatorMetadataGenerator.generateOperatorJsonSchema(opClass), + "$" + ) + ) + .filter { case (_, rules) => rules.nonEmpty } + .sortBy { case (operator, _) => operator } + + // Without this the per-operator cases below would all vanish, and the suite would + // pass by testing nothing. + "Some operator" should s"declare $RulesKeyword, so the cases below are not vacuous" in { + rulesByOperator should not be empty + } + + rulesByOperator.foreach { + case (operator, rules) => + behavior of s"The $RulesKeyword on $operator" + + it should "name only properties the generated schema declares" in { + val unmatched = rules.filterNot(rule => rule.declaredProperties.contains(rule.key)) + withClue("rules naming no property: ")(unmatched shouldBe empty) + } + + it should "state each rule as an object, so the constraint is readable" in { + val notObjects = rules.filterNot(_.value.isObject) + withClue("rules that are not objects: ")(notObjects shouldBe empty) + } + } +}
