Copilot commented on code in PR #4268:
URL: https://github.com/apache/texera/pull/4268#discussion_r2907501231
##########
amber/src/main/python/pytexera/udf/udf_operator.py:
##########
@@ -16,12 +16,76 @@
# under the License.
from abc import abstractmethod
-from typing import Iterator, Optional, Union
-
-from pyamber import *
+from typing import Any, Dict, Iterator, Optional, Union
+import functools
-class UDFOperatorV2(TupleOperatorV2):
+from pyamber import *
+from core.models.schema.attribute_type import AttributeType,
FROM_STRING_PARSER_MAPPING
+
+class _UiParameterSupport:
+ _ui_parameter_injected_values: Dict[str, Any] = {}
+ _ui_parameter_name_types: Dict[str, AttributeType] = {}
+
+ # Reserved hook name. Backend injector will generate this in the user's
class.
+ def _texera_injected_ui_parameters(self) -> Dict[str, Any]:
+ return {}
+
+ def _texera_apply_injected_ui_parameters(self) -> None:
+ values = self._texera_injected_ui_parameters()
+ # Write to base class storage (not cls) because UiParameter reads from
_UiParameterSupport directly
+ _UiParameterSupport._ui_parameter_injected_values = dict(values or {})
+ _UiParameterSupport._ui_parameter_name_types = {}
Review Comment:
`_ui_parameter_injected_values` and `_ui_parameter_name_types` are
class-level dictionaries on `_UiParameterSupport` (lines 27-28). This means all
subclass instances share the same state. If multiple UDF operator instances run
concurrently in the same Python worker process (e.g., due to parallelism), one
operator's `_texera_apply_injected_ui_parameters()` call could overwrite
another operator's injected values, causing incorrect parameter values to be
read. Each operator instance should have its own storage, for example by using
instance-level variables instead of class-level ones.
##########
frontend/src/app/workspace/service/code-editor/ui-udf-parameters-parser.service.spec.ts:
##########
@@ -0,0 +1,79 @@
+/**
+ * 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.
+ */
+
+import { UiUdfParametersParserService } from
"./ui-udf-parameters-parser.service";
+
+describe("UiUdfParametersParserService", () => {
+ let service: UiUdfParametersParserService;
+
+ beforeEach(() => {
+ service = new UiUdfParametersParserService();
+ });
+
+ it("should parse positional and name-based arguments", () => {
+ const code = `
+ class ProcessTupleOperator(UDFOperatorV2):
+ def open(self):
+ self.UiParameter(AttributeType.INT, "count")
+ self.UiParameter(type=AttributeType.STRING, name="name")
+ self.UiParameter(name="age", type=AttributeType.LONG)
+ self.UiParameter(AttributeType.DOUBLE, name="score")
+ self.UiParameter("created_at", type=AttributeType.TIMESTAMP)
+ `;
Review Comment:
There is a mismatch between the test code calling convention and the actual
Python `UiParameter` signature. The test uses
`self.UiParameter(AttributeType.INT, "count")` — type first, name second.
However, the Python `UiParameter.__init__` signature is `def __init__(self,
name: str, type: AttributeType)` (name first, type second). If a user follows
this positional call pattern from the test, calling
`self.UiParameter(AttributeType.INT, "count")` in Python would fail with a
`TypeError` since `AttributeType.INT` would be passed as `name` and `"count"`
as `type`. While the frontend parser correctly handles both orderings (since it
detects positional tokens by pattern matching), the test inadvertently
documents an unsupported Python call pattern. The test should use the correct
Python positional call convention, or strictly use keyword arguments.
##########
frontend/src/app/workspace/component/ui-udf-parameters/ui-udf-parameters.component.html:
##########
@@ -0,0 +1,55 @@
+<!--
+ 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.
+-->
+<div
+ class="ui-udf-param-list"
+ *ngIf="(model?.length ?? 0) > 0">
+ <!-- Optional header row -->
+ <div class="ui-udf-param-row header">
+ <div class="field-cell"><span class="col-title">Value</span></div>
+ <div class="field-cell"><span class="col-title">Name</span></div>
+ <div class="field-cell"><span class="col-title">Type</span></div>
+ </div>
+
+ <div
+ class="ui-udf-param-row"
+ *ngFor="let param of (model || []); let i = index; trackBy:
trackByParamName">
+ <ng-container *ngIf="field.fieldGroup?.[i] as rowField">
+ <!-- Value -->
+ <div class="field-cell">
+ <ng-container *ngIf="getValueField(rowField) as valueField">
+ <formly-field [field]="valueField"></formly-field>
+ </ng-container>
+ </div>
+
+ <!-- Name -->
+ <div class="field-cell">
+ <ng-container *ngIf="getNameField(rowField) as nameField">
+ <formly-field [field]="nameField"></formly-field>
+ </ng-container>
+ </div>
+
+ <!-- Type -->
+ <div class="field-cell">
+ <ng-container *ngIf="getTypeField(rowField) as typeField">
+ <formly-field [field]="typeField"></formly-field>
+ </ng-container>
+ </div>
+ </ng-container>
+ </div>
+</div>
Review Comment:
The column order in the UI template renders "Value", "Name", "Type" (left to
right) across both the header row and data rows. However, the PR description
screenshot shows the column order as "Name", "Type", "Value". This is a
discrepancy between the PR description and the actual code. The PR description
screenshot likely shows the intended UX (Name and Type are read-only attributes
that describe the parameter, while Value is what users edit). The current code
places the editable "Value" column first, which may be confusing since users
typically scan left-to-right and seeing Value before knowing the Name/Type of
the parameter is less intuitive.
##########
frontend/src/app/workspace/component/code-editor-dialog/code-editor.component.ts:
##########
@@ -47,11 +47,12 @@ import "@codingame/monaco-vscode-python-default-extension";
import "@codingame/monaco-vscode-r-default-extension";
import "@codingame/monaco-vscode-java-default-extension";
import { isDefined } from "../../../common/util/predicate";
-import { filter, switchMap } from "rxjs/operators";
+import { debounceTime, filter, switchMap } from "rxjs/operators";
Review Comment:
`debounceTime` is imported but never used in this file. The commented-out
`pythonCodeChangeSubject` field on line 106 suggests this was intended for
throttling code change events, but the implementation was changed to use
`yCode.observe(handler)` directly without debouncing. The unused import should
be removed to keep the code clean.
##########
frontend/src/app/common/formly/collab-wrapper/collab-wrapper/collab-wrapper.component.css:
##########
@@ -25,7 +25,7 @@
outline: transparent;
overflow: visible;
position: relative;
- : 1pt;
+ :1pt;
Review Comment:
The CSS rule on line 28 is invalid: `:1pt;` is not a valid CSS declaration —
it has no property name. This appears to be a stray/broken rule. The original
code before this change was ` : 1pt;` which was also invalid, but the
refactoring of removing the leading space didn't fix it. This line should
either be removed entirely or corrected to a valid CSS property (e.g., `border:
1pt;`).
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/udf/python/PythonUdfUiParameterInjector.scala:
##########
@@ -0,0 +1,196 @@
+/*
+ * 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.udf.python
+
+import org.apache.texera.amber.pybuilder.PythonTemplateBuilder
+import
org.apache.texera.amber.pybuilder.PythonTemplateBuilder.PythonTemplateBuilderStringContext
+
+import scala.util.matching.Regex
+
+object PythonUdfUiParameterInjector {
+
+ private val ReservedHookMethod = "_texera_injected_ui_parameters"
+
+ // Match user-facing UDF classes (the ones users write)
+ private val SupportedUserClassRegex: Regex =
+ """(?m)^([
\t]*)class\s+(ProcessTupleOperator|ProcessBatchOperator|ProcessTableOperator|GenerateOperator)\s*\([^)]*\)\s*:\s*(?:#.*)?$""".r
+
+ private def validate(uiParameters: List[UiUDFParameter]): Unit = {
+ uiParameters.foreach { parameter =>
+ if (parameter.attribute == null) {
+ throw new RuntimeException("UiParameter attribute is required.")
+ }
+ }
+
+ val grouped = uiParameters.groupBy(_.attribute.getName)
+ grouped.foreach {
+ case (key, values) =>
+ val typeSet = values.map(_.attribute.getType).toSet
+ if (typeSet.size > 1) {
+ throw new RuntimeException(
+ s"UiParameter key '$key' has multiple types:
${typeSet.map(_.name()).mkString(",")}."
+ )
+ }
+ }
+ }
+
+ private def buildInjectedParametersMap(
+ uiParameters: List[UiUDFParameter]
+ ): PythonTemplateBuilder = {
+ val entries = uiParameters.map { parameter =>
+ pyb"'${parameter.attribute.getName()}': ${parameter.value}"
+ }
+
+ entries.reduceOption((acc, entry) => acc + pyb", " +
entry).getOrElse(pyb"")
+ }
+
+ private def buildInjectedHookMethod(uiParameters: List[UiUDFParameter]):
String = {
+ val injectedParametersMap = buildInjectedParametersMap(uiParameters)
+
+ // unindented method; we indent it when inserting into the class body
+ (pyb"""|@overrides
+ |def """ + pyb"$ReservedHookMethod" + pyb"""(self) ->
typing.Dict[str, typing.Any]:
+ | return {""" +
+ injectedParametersMap +
+ pyb"""}
+ |""").encode
+ }
+
+ private def ensureTypingImport(encodedUserCode: String): String = {
+ val alreadyImported = encodedUserCode
+ .split("\n", -1)
+ .exists(_.trim == "import typing")
+
+ if (alreadyImported) {
+ encodedUserCode
+ } else {
+ "import typing\n" + encodedUserCode
+ }
+ }
+
+ private def indentBlock(block: String, indent: String): String = {
+ block
+ .split("\n", -1)
+ .map { line =>
+ if (line.nonEmpty) indent + line else line
+ }
+ .mkString("\n")
+ }
+
+ private def lineEndIndex(text: String, from: Int): Int = {
+ val idx = text.indexOf('\n', from)
+ if (idx < 0) text.length else idx
+ }
+
+ private def detectClassBlockEnd(code: String, classHeaderStart: Int,
classIndent: String): Int = {
+ val classLineEnd = lineEndIndex(code, classHeaderStart)
+ var pos = if (classLineEnd < code.length) classLineEnd + 1 else code.length
+
+ while (pos < code.length) {
+ val end = lineEndIndex(code, pos)
+ val line = code.substring(pos, end)
+
+ val trimmed = line.trim
+ val isBlank = trimmed.isEmpty
+
+ // a top-level (or same/lower-indented) non-blank line ends the class
block
+ val currentIndentLen = line.prefixLength(ch => ch == ' ' || ch == '\t')
+ val classIndentLen = classIndent.length
+
+ if (!isBlank && currentIndentLen <= classIndentLen) {
+ return pos
+ }
+
+ pos = if (end < code.length) end + 1 else code.length
+ }
+
+ code.length
+ }
+
+ private def containsReservedHook(classBlock: String): Boolean = {
+ val hookRegex = ("""(?m)^[ \t]+def\s+""" + Regex.quote(ReservedHookMethod)
+ """\s*\(""").r
+ hookRegex.findFirstIn(classBlock).isDefined
+ }
+
+ private def findInsertionPointInsideClass(classBlock: String, classIndent:
String): Int = {
+ // Insert before the first method definition in the class body.
+ // This preserves existing open() and also preserves class docstrings if
present.
+ val methodRegex = """(?m)^[ \t]+def\s+\w+\s*\(""".r
+
methodRegex.findFirstMatchIn(classBlock).map(_.start).getOrElse(classBlock.length)
+ }
Review Comment:
The `findInsertionPointInsideClass` method is defined on line 131 but is
never called anywhere in the file. The `injectHookIntoUserClass` method instead
always appends the injected hook at the end of the class block
(`detectClassBlockEnd` is used) rather than before the first method. This dead
method should be removed to avoid confusion.
##########
common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/udf/python/PythonUdfUiParameterInjectorSpec.scala:
##########
@@ -0,0 +1,181 @@
+/*
+ * 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.udf.python
+
+import org.apache.texera.amber.core.tuple.{Attribute, AttributeType}
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+
+class PythonUdfUiParameterInjectorSpec extends AnyFlatSpec with Matchers {
+
+ private def createParameter(
+ key: String,
+ attributeType: AttributeType,
+ value: String
+ ): UiUDFParameter = {
+ val parameter = new UiUDFParameter
+ parameter.attribute = new Attribute(key, attributeType)
+ parameter.value = value
+ parameter
+ }
+
+ private val baseUdfCode: String =
+ """from pytexera import *
+ |
+ |class ProcessTupleOperator(UDFOperatorV2):
+ | @overrides
+ | def open(self):
+ | print("open")
+ |
+ | @overrides
+ | def process_tuple(self, tuple_: Tuple, port: int):
+ | yield tuple_
+ |""".stripMargin
+
+ it should "return encoded user code unchanged when there are no ui
parameters" in {
+ val injectedCode = PythonUdfUiParameterInjector.inject(baseUdfCode, Nil)
+
+ injectedCode should include("class ProcessTupleOperator(UDFOperatorV2):")
+ injectedCode should include("""print("open")""")
+ injectedCode should not include ("_texera_injected_ui_parameters")
+ injectedCode should not include ("self.decode_python_template")
+ }
+
+ it should "inject ui parameter hook into supported UDF class" in {
+ val injectedCode = PythonUdfUiParameterInjector.inject(
+ baseUdfCode,
+ List(
+ createParameter("date", AttributeType.TIMESTAMP,
"2024-01-01T00:00:00Z")
+ )
+ )
+
+ injectedCode should include("class ProcessTupleOperator(UDFOperatorV2):")
+ injectedCode should include("def _texera_injected_ui_parameters(self):")
+ injectedCode should include("return {")
+ injectedCode should include("self.decode_python_template")
+ injectedCode should include("""print("open")""")
+ }
+
+ it should "inject the reserved hook before the first method definition" in {
+ val injectedCode = PythonUdfUiParameterInjector.inject(
+ baseUdfCode,
+ List(createParameter("k", AttributeType.STRING, "v"))
+ )
+
+ val hookIndex = injectedCode.indexOf("def
_texera_injected_ui_parameters(self):")
+ val openIndex = injectedCode.indexOf("def open(self):")
+
+ hookIndex should be >= 0
+ openIndex should be > hookIndex
+ }
Review Comment:
The test has two issues:
1. `hookIndex = injectedCode.indexOf("def
_texera_injected_ui_parameters(self):")` will always return `-1` because the
injected method signature includes a return type annotation: `def
_texera_injected_ui_parameters(self) -> typing.Dict[str, typing.Any]:`. The
substring `(self):` is not present. The assertion `hookIndex should be >= 0`
will fail.
2. Even if the string search were fixed, `injectHookIntoUserClass` appends
the hook at `classBlockEnd` (end of class body), placing it *after* `def
open(self):`. This means `hookIndex > openIndex`, so the assertion `openIndex
should be > hookIndex` would also fail. The dead method
`findInsertionPointInsideClass` (line 131-136) was apparently intended to solve
this by inserting before the first method, but it is never called.
##########
amber/src/main/python/pytexera/udf/udf_operator.py:
##########
@@ -16,12 +16,76 @@
# under the License.
from abc import abstractmethod
-from typing import Iterator, Optional, Union
-
-from pyamber import *
+from typing import Any, Dict, Iterator, Optional, Union
+import functools
-class UDFOperatorV2(TupleOperatorV2):
+from pyamber import *
+from core.models.schema.attribute_type import AttributeType,
FROM_STRING_PARSER_MAPPING
+
+class _UiParameterSupport:
+ _ui_parameter_injected_values: Dict[str, Any] = {}
+ _ui_parameter_name_types: Dict[str, AttributeType] = {}
+
+ # Reserved hook name. Backend injector will generate this in the user's
class.
+ def _texera_injected_ui_parameters(self) -> Dict[str, Any]:
+ return {}
+
+ def _texera_apply_injected_ui_parameters(self) -> None:
+ values = self._texera_injected_ui_parameters()
+ # Write to base class storage (not cls) because UiParameter reads from
_UiParameterSupport directly
+ _UiParameterSupport._ui_parameter_injected_values = dict(values or {})
+ _UiParameterSupport._ui_parameter_name_types = {}
+
+ def __init_subclass__(cls, **kwargs):
+ super().__init_subclass__(**kwargs)
+
+ # Wrap only methods defined on this class (not inherited ones)
+ original_open = getattr(cls, "open", None)
+ if original_open is None:
+ return
+
+ # Avoid double wrapping
+ if getattr(original_open, "__texera_ui_params_wrapped__", False):
+ return
+
+ @functools.wraps(original_open)
+ def wrapped_open(self, *args, **kwargs):
+ self._texera_apply_injected_ui_parameters()
+ return original_open(self, *args, **kwargs)
+
+ setattr(wrapped_open, "__texera_ui_params_wrapped__", True)
+ cls.open = wrapped_open
+
+ class UiParameter:
+ def __init__(self, name: str, type: AttributeType):
+ if not isinstance(type, AttributeType):
+ raise TypeError(
+ f"UiParameter.type must be an AttributeType, got {type!r}."
+ )
+
+ existing_type =
_UiParameterSupport._ui_parameter_name_types.get(name)
+ if existing_type is not None and existing_type != type:
+ raise ValueError(
+ f"Duplicate UiParameter name '{name}' with conflicting
types: "
+ f"{existing_type.name} vs {type.name}."
+ )
+
+ _UiParameterSupport._ui_parameter_name_types[name] = type
+ raw_value =
_UiParameterSupport._ui_parameter_injected_values.get(name)
+ self.name = name
+ self.type = type
+ self.value = _UiParameterSupport._parse(raw_value, type)
Review Comment:
The `UiParameter.__init__` method uses `type` as a parameter name (line 61),
which shadows the Python built-in `type()` function. While this works correctly
inside the function, it is a Python anti-pattern that can lead to subtle bugs
if developers modify this code and inadvertently try to use `type()`. A clearer
name like `attr_type` would be more consistent with `_parse`'s parameter name
`attr_type` used on line 81.
##########
common/workflow-core/src/main/scala/org/apache/texera/amber/core/tuple/Attribute.java:
##########
@@ -21,6 +21,10 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.texera.amber.pybuilder.EncodableStringAnnotation;
+import org.apache.texera.amber.pybuilder.PyStringTypes;
+import org.apache.texera.amber.pybuilder.PyStringTypes.EncodableStringFactory$;
Review Comment:
Two unused imports were added: `import
org.apache.texera.amber.pybuilder.PyStringTypes;` (line 25) and `import
org.apache.texera.amber.pybuilder.PyStringTypes.EncodableStringFactory$;` (line
26). Neither `PyStringTypes` nor `EncodableStringFactory$` are referenced
anywhere in the file body — only `@EncodableStringAnnotation` from line 24's
import is used. These unused imports should be removed.
##########
amber/src/main/python/pytexera/udf/udf_operator.py:
##########
@@ -16,12 +16,76 @@
# under the License.
from abc import abstractmethod
-from typing import Iterator, Optional, Union
-
-from pyamber import *
+from typing import Any, Dict, Iterator, Optional, Union
+import functools
-class UDFOperatorV2(TupleOperatorV2):
+from pyamber import *
+from core.models.schema.attribute_type import AttributeType,
FROM_STRING_PARSER_MAPPING
+
+class _UiParameterSupport:
+ _ui_parameter_injected_values: Dict[str, Any] = {}
+ _ui_parameter_name_types: Dict[str, AttributeType] = {}
+
+ # Reserved hook name. Backend injector will generate this in the user's
class.
+ def _texera_injected_ui_parameters(self) -> Dict[str, Any]:
+ return {}
+
+ def _texera_apply_injected_ui_parameters(self) -> None:
+ values = self._texera_injected_ui_parameters()
+ # Write to base class storage (not cls) because UiParameter reads from
_UiParameterSupport directly
+ _UiParameterSupport._ui_parameter_injected_values = dict(values or {})
+ _UiParameterSupport._ui_parameter_name_types = {}
+
+ def __init_subclass__(cls, **kwargs):
+ super().__init_subclass__(**kwargs)
+
+ # Wrap only methods defined on this class (not inherited ones)
+ original_open = getattr(cls, "open", None)
+ if original_open is None:
+ return
+
+ # Avoid double wrapping
+ if getattr(original_open, "__texera_ui_params_wrapped__", False):
+ return
+
+ @functools.wraps(original_open)
+ def wrapped_open(self, *args, **kwargs):
+ self._texera_apply_injected_ui_parameters()
+ return original_open(self, *args, **kwargs)
+
+ setattr(wrapped_open, "__texera_ui_params_wrapped__", True)
+ cls.open = wrapped_open
+
+ class UiParameter:
+ def __init__(self, name: str, type: AttributeType):
+ if not isinstance(type, AttributeType):
+ raise TypeError(
+ f"UiParameter.type must be an AttributeType, got {type!r}."
+ )
+
+ existing_type =
_UiParameterSupport._ui_parameter_name_types.get(name)
+ if existing_type is not None and existing_type != type:
+ raise ValueError(
+ f"Duplicate UiParameter name '{name}' with conflicting
types: "
+ f"{existing_type.name} vs {type.name}."
+ )
+
+ _UiParameterSupport._ui_parameter_name_types[name] = type
+ raw_value =
_UiParameterSupport._ui_parameter_injected_values.get(name)
+ self.name = name
+ self.type = type
+ self.value = _UiParameterSupport._parse(raw_value, type)
+
+ @staticmethod
+ def _parse(value: Any, attr_type: AttributeType) -> Any:
+ if value is None:
+ return None
+
+ py_type = FROM_STRING_PARSER_MAPPING.get(attr_type)
+ return py_type(value)
+
Review Comment:
The new `_UiParameterSupport` class and `UiParameter` inner class in
`udf_operator.py` have no corresponding Python unit tests. The surrounding
codebase has thorough test coverage (e.g., `test_echo_operator.py`,
`test_count_batch_operator.py`). Tests should be added to cover at minimum:
parameter value injection via `_texera_apply_injected_ui_parameters`, duplicate
name detection with conflicting types in `UiParameter`, the `_parse` method for
all supported attribute types, and the `__init_subclass__` wrapping behavior.
##########
frontend/src/app/workspace/component/code-editor-dialog/code-editor.component.ts:
##########
@@ -102,6 +103,7 @@ export class CodeEditorComponent implements AfterViewInit,
SafeStyle, OnDestroy
private isMultipleVariables: boolean = false;
public codeDebuggerComponent!: Type<any> | null;
public editorToPass!: MonacoEditor;
+ // private readonly pythonCodeChangeSubject = new Subject<string>();
Review Comment:
The commented-out field `// private readonly pythonCodeChangeSubject = new
Subject<string>();` on line 106 appears to be leftover dead code from a
previous implementation approach. It should be removed to keep the file clean.
##########
frontend/src/app/workspace/service/code-editor/ui-udf-parameters-sync.service.ts:
##########
@@ -0,0 +1,131 @@
+/**
+ * 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.
+ */
+import { Injectable } from "@angular/core";
+import { isEqual } from "lodash-es";
+import { ReplaySubject } from "rxjs";
+import { Subject } from "rxjs";
Review Comment:
The `Subject` import from `"rxjs"` on line 22 is unused. Only
`ReplaySubject` is actually used in this file. This unused import should be
removed.
##########
amber/src/main/python/core/models/schema/attribute_type.py:
##########
@@ -78,6 +77,17 @@ class AttributeType(Enum):
}
+FROM_STRING_PARSER_MAPPING = {
+ AttributeType.STRING: str,
+ AttributeType.INT: int,
+ AttributeType.LONG: int,
+ AttributeType.DOUBLE: float,
+ AttributeType.BOOL: lambda v: str(v).strip().lower() in ("True", "true",
"1", "yes"),
Review Comment:
The boolean parser in `FROM_STRING_PARSER_MAPPING` for `AttributeType.BOOL`
includes `"True"` in the truthy set (line 85), but this is redundant since the
lambda already calls `str(v).strip().lower()` — after `.lower()`, no string can
equal `"True"` (capital T). The `"True"` entry in the tuple is dead code and
the set effectively only checks for `"true"`, `"1"`, and `"yes"`.
##########
amber/src/main/python/pytexera/udf/udf_operator.py:
##########
@@ -16,12 +16,76 @@
# under the License.
from abc import abstractmethod
-from typing import Iterator, Optional, Union
-
-from pyamber import *
+from typing import Any, Dict, Iterator, Optional, Union
+import functools
-class UDFOperatorV2(TupleOperatorV2):
+from pyamber import *
+from core.models.schema.attribute_type import AttributeType,
FROM_STRING_PARSER_MAPPING
+
+class _UiParameterSupport:
+ _ui_parameter_injected_values: Dict[str, Any] = {}
+ _ui_parameter_name_types: Dict[str, AttributeType] = {}
+
+ # Reserved hook name. Backend injector will generate this in the user's
class.
+ def _texera_injected_ui_parameters(self) -> Dict[str, Any]:
+ return {}
+
+ def _texera_apply_injected_ui_parameters(self) -> None:
+ values = self._texera_injected_ui_parameters()
+ # Write to base class storage (not cls) because UiParameter reads from
_UiParameterSupport directly
+ _UiParameterSupport._ui_parameter_injected_values = dict(values or {})
+ _UiParameterSupport._ui_parameter_name_types = {}
+
+ def __init_subclass__(cls, **kwargs):
+ super().__init_subclass__(**kwargs)
+
+ # Wrap only methods defined on this class (not inherited ones)
+ original_open = getattr(cls, "open", None)
+ if original_open is None:
+ return
+
+ # Avoid double wrapping
+ if getattr(original_open, "__texera_ui_params_wrapped__", False):
+ return
+
+ @functools.wraps(original_open)
+ def wrapped_open(self, *args, **kwargs):
+ self._texera_apply_injected_ui_parameters()
+ return original_open(self, *args, **kwargs)
+
+ setattr(wrapped_open, "__texera_ui_params_wrapped__", True)
+ cls.open = wrapped_open
+
+ class UiParameter:
+ def __init__(self, name: str, type: AttributeType):
+ if not isinstance(type, AttributeType):
+ raise TypeError(
+ f"UiParameter.type must be an AttributeType, got {type!r}."
+ )
+
+ existing_type =
_UiParameterSupport._ui_parameter_name_types.get(name)
+ if existing_type is not None and existing_type != type:
+ raise ValueError(
+ f"Duplicate UiParameter name '{name}' with conflicting
types: "
+ f"{existing_type.name} vs {type.name}."
+ )
+
+ _UiParameterSupport._ui_parameter_name_types[name] = type
+ raw_value =
_UiParameterSupport._ui_parameter_injected_values.get(name)
+ self.name = name
+ self.type = type
+ self.value = _UiParameterSupport._parse(raw_value, type)
+
+ @staticmethod
+ def _parse(value: Any, attr_type: AttributeType) -> Any:
+ if value is None:
+ return None
+
+ py_type = FROM_STRING_PARSER_MAPPING.get(attr_type)
+ return py_type(value)
Review Comment:
The `_parse` method in `_UiParameterSupport` (lines 81-86) does not handle
the case where `FROM_STRING_PARSER_MAPPING.get(attr_type)` returns `None`
(i.e., when the `attr_type` is not in the mapping). If `py_type` is `None`,
calling `py_type(value)` on line 86 will raise a `TypeError: 'NoneType' object
is not callable`, giving an unhelpful error to the user.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
