Copilot commented on code in PR #5896:
URL: https://github.com/apache/texera/pull/5896#discussion_r3508555424
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/aggregate/AggregateOpExec.scala:
##########
@@ -47,9 +47,14 @@ class AggregateOpExec(descString: String) extends
OperatorExecutor {
// Initialize distributedAggregations if it's not yet initialized
if (distributedAggregations == null) {
- distributedAggregations = desc.aggregations.map(agg =>
- agg.getAggFunc(tuple.getSchema.getAttribute(agg.attribute).getType)
- )
+ distributedAggregations = desc.aggregations.map { agg =>
+ // An empty attribute (COUNT(*)) has no input column to look up;
COUNT's result
+ // type does not depend on any input attribute, so a null attrType is
safe here.
+ val attrType =
+ if (agg.attribute == null || agg.attribute.trim.isEmpty) null
+ else tuple.getSchema.getAttribute(agg.attribute).getType
+ agg.getAggFunc(attrType)
Review Comment:
`attrType` is treated as nullable whenever `attribute` is blank, regardless
of which aggregation function is selected. That makes it possible for an
invalid non-COUNT config (e.g., SUM with an empty attribute via API / stale
workflow JSON) to pass a null `attrType` into `getAggFunc`, which can produce
misleading errors (and in some cases propagate a null output type).
Since only COUNT(*) is intended to tolerate a blank attribute, key this
guard off `agg.aggFunction == COUNT` (and blank attribute) rather than blank
attribute alone, and otherwise always look up the schema type (letting it fail
fast if the attribute is missing/invalid).
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/aggregate/AggregateOpDesc.scala:
##########
@@ -78,9 +78,14 @@ class AggregateOpDesc extends LogicalOp {
val inputSchema = inputSchemas(operatorInfo.inputPorts.head.id)
val outputSchema = Schema(
groupByKeys.map(key => inputSchema.getAttribute(key)) ++
- localAggregations.map(agg =>
-
agg.getAggregationAttribute(inputSchema.getAttribute(agg.attribute).getType)
- )
+ localAggregations.map { agg =>
+ // An empty attribute (COUNT(*)) has no input column to look
up; COUNT's
+ // result type is INTEGER regardless, so a null attrType is
safe here.
+ val attrType =
+ if (agg.attribute == null || agg.attribute.trim.isEmpty) null
+ else inputSchema.getAttribute(agg.attribute).getType
+ agg.getAggregationAttribute(attrType)
Review Comment:
Schema propagation currently treats a blank `attribute` as “no input column”
for *all* aggregation functions and passes a null `attrType` into
`getAggregationAttribute`. For non-COUNT functions, that can yield an output
Attribute with a null type (e.g., SUM/MIN/MAX return `attrType`), which can
break downstream schema consumers.
Only COUNT(*) should allow a blank attribute; for all other functions,
schema propagation should always resolve the input attribute type (or fail fast
if the attribute is missing/invalid).
##########
frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts:
##########
@@ -77,6 +77,11 @@ import { map, switchMap, take } from "rxjs/operators";
Quill.register("modules/cursors", QuillCursors);
+// The Aggregate "count" function. With an empty attribute it means COUNT(*)
(all rows);
+// with a column it counts that column's non-null values. It is the only
function whose
+// attribute is optional.
+export const AGGREGATE_COUNT = "count";
Review Comment:
The PR description says a dedicated `count(*)` aggregation function (e.g.
`COUNT_STAR("count(*)")`) was added alongside the existing `count`. In the
actual code shown here, the UI and docs treat **`count` with an empty
attribute** as `COUNT(*)` (and `AggregationFunction` still only contains
`COUNT`).
Please either update the PR description to match the implemented API/UX
(COUNT(*) via empty attribute on `count`) or implement the separate `count(*)`
function throughout (backend enum/schema + frontend selection) so the PR
matches its stated behavior.
--
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]
