MartijnVisser commented on code in PR #24443: URL: https://github.com/apache/flink/pull/24443#discussion_r4432258382
########## flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/aggregate/ModeAggFunction.java: ########## @@ -0,0 +1,175 @@ +/* + * 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.flink.table.runtime.functions.aggregate; + +import org.apache.flink.annotation.Internal; +import org.apache.flink.table.api.DataTypes; +import org.apache.flink.table.api.dataview.MapView; +import org.apache.flink.table.types.DataType; +import org.apache.flink.table.types.logical.LogicalType; + +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Objects; + +import static org.apache.flink.table.types.utils.DataTypeUtils.toInternalDataType; + +/** Built-in MODE with retraction aggregate function. */ +@Internal +public class ModeAggFunction<T> extends BuiltInAggregateFunction<T, ModeAggFunction.ModeAcc<T>> { + + private final transient DataType valueDataType; + + public ModeAggFunction(LogicalType valueType) { + this.valueDataType = toInternalDataType(valueType); + } + + @Override + public List<DataType> getArgumentDataTypes() { + return Collections.singletonList(valueDataType); + } + + @Override + public DataType getOutputDataType() { + return valueDataType; + } + + @Override + public T getValue(ModeAcc<T> accumulator) { + return accumulator.curMode; + } + + @Override + public ModeAcc<T> createAccumulator() { + final ModeAcc<T> acc = new ModeAcc<>(); + acc.buffer = new MapView<>(); + acc.curCnt = 0; + acc.curMode = null; + return acc; + } + + @Override + public DataType getAccumulatorDataType() { + return DataTypes.STRUCTURED( + ModeAcc.class, + DataTypes.FIELD("curCnt", DataTypes.BIGINT()), + DataTypes.FIELD("curMode", valueDataType.nullable()), + DataTypes.FIELD( + "buffer", + MapView.newMapViewDataType(valueDataType.notNull(), DataTypes.BIGINT()))); + } + + /** Accumulator for MODE. */ + public static class ModeAcc<T> { + public long curCnt; + public T curMode; + public MapView<T, Long> buffer; + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + ModeAcc<?> lagAcc = (ModeAcc<?>) o; + return curCnt == lagAcc.curCnt + && Objects.equals(curMode, lagAcc.curMode) + && Objects.equals(buffer, lagAcc.buffer); + } + + @Override + public int hashCode() { + return Objects.hash(curCnt, curMode, buffer); + } + } + + public void accumulate(ModeAcc<T> acc, T value) throws Exception { + if (value == null) { + return; + } + Long cnt = acc.buffer.get(value); + cnt = cnt == null ? 1 : cnt + 1; + acc.buffer.put(value, cnt); + if (Objects.equals(value, acc.curMode)) { + acc.curCnt = cnt; + } else if (cnt > acc.curCnt) { + acc.curMode = value; + acc.curCnt = cnt; + } + } + + public void retract(ModeAcc<T> acc, T value) throws Exception { + if (value == null) { + return; + } + Long cnt = acc.buffer.get(value); + cnt = cnt == null ? 0L : cnt - 1; Review Comment: A retraction on an empty accumulator is dropped here, which a local aggregate hits with mini-batch and `TWO_PHASE`. I think counts need to go negative, like in `MaxWithRetractAggFunction`. ########## flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/aggfunctions/ModeAggFunctionTest.java: ########## @@ -0,0 +1,74 @@ +/* + * 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.flink.table.planner.functions.aggfunctions; + +import org.apache.flink.table.data.StringData; +import org.apache.flink.table.runtime.functions.aggregate.ModeAggFunction; +import org.apache.flink.table.types.logical.VarCharType; + +import java.lang.reflect.Method; +import java.util.Arrays; +import java.util.List; + +import static org.apache.flink.table.data.StringData.fromString; + +/** Test case for built-in MODE with retraction aggregate function. */ +final class ModeAggFunctionTest + extends AggFunctionTestBase<StringData, StringData, ModeAggFunction.ModeAcc<StringData>> { + + @Override + protected List<List<StringData>> getInputValueSets() { + return Arrays.asList( + Arrays.asList(fromString("1"), fromString("1"), fromString("3")), + Arrays.asList(fromString("4"), null, fromString("4")), + Arrays.asList(null, null), + Arrays.asList( Review Comment: The skip is needed, `subList(0, 2)` throws on the `[null]` input. This test would also catch the dropped retractions with an input like 1, 1, 3, 3, 3, 3, 3. ########## flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/aggregate/ModeAggFunction.java: ########## @@ -0,0 +1,175 @@ +/* + * 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.flink.table.runtime.functions.aggregate; + +import org.apache.flink.annotation.Internal; +import org.apache.flink.table.api.DataTypes; +import org.apache.flink.table.api.dataview.MapView; +import org.apache.flink.table.types.DataType; +import org.apache.flink.table.types.logical.LogicalType; + +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Objects; + +import static org.apache.flink.table.types.utils.DataTypeUtils.toInternalDataType; + +/** Built-in MODE with retraction aggregate function. */ +@Internal +public class ModeAggFunction<T> extends BuiltInAggregateFunction<T, ModeAggFunction.ModeAcc<T>> { + + private final transient DataType valueDataType; + + public ModeAggFunction(LogicalType valueType) { + this.valueDataType = toInternalDataType(valueType); + } + + @Override + public List<DataType> getArgumentDataTypes() { + return Collections.singletonList(valueDataType); + } + + @Override + public DataType getOutputDataType() { + return valueDataType; + } + + @Override + public T getValue(ModeAcc<T> accumulator) { + return accumulator.curMode; + } + + @Override + public ModeAcc<T> createAccumulator() { + final ModeAcc<T> acc = new ModeAcc<>(); + acc.buffer = new MapView<>(); + acc.curCnt = 0; + acc.curMode = null; + return acc; + } + + @Override + public DataType getAccumulatorDataType() { + return DataTypes.STRUCTURED( + ModeAcc.class, + DataTypes.FIELD("curCnt", DataTypes.BIGINT()), + DataTypes.FIELD("curMode", valueDataType.nullable()), + DataTypes.FIELD( + "buffer", + MapView.newMapViewDataType(valueDataType.notNull(), DataTypes.BIGINT()))); + } + + /** Accumulator for MODE. */ + public static class ModeAcc<T> { + public long curCnt; + public T curMode; + public MapView<T, Long> buffer; + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + ModeAcc<?> lagAcc = (ModeAcc<?>) o; + return curCnt == lagAcc.curCnt + && Objects.equals(curMode, lagAcc.curMode) + && Objects.equals(buffer, lagAcc.buffer); + } + + @Override + public int hashCode() { + return Objects.hash(curCnt, curMode, buffer); + } + } + + public void accumulate(ModeAcc<T> acc, T value) throws Exception { + if (value == null) { + return; + } + Long cnt = acc.buffer.get(value); + cnt = cnt == null ? 1 : cnt + 1; + acc.buffer.put(value, cnt); + if (Objects.equals(value, acc.curMode)) { + acc.curCnt = cnt; + } else if (cnt > acc.curCnt) { + acc.curMode = value; + acc.curCnt = cnt; + } + } + + public void retract(ModeAcc<T> acc, T value) throws Exception { + if (value == null) { + return; + } + Long cnt = acc.buffer.get(value); + cnt = cnt == null ? 0L : cnt - 1; + if (cnt <= 0) { + acc.buffer.remove(value); + } else { + acc.buffer.put(value, cnt); + } + acc.curCnt = cnt; Review Comment: This should only happen when `value` is the current mode. Inserting 1, 1, 1, 2 and then deleting 2 returns NULL instead of 1. ########## flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/BaseExpressions.java: ########## @@ -502,6 +503,15 @@ public OutType lastValue() { return toApiSpecificExpression(unresolvedCall(LAST_VALUE, toExpr())); } + /** + * Returns the most frequent value in a group of values. If there are multiple values that + * appear the same number of times, one of them will be returned. NULL values are ignored. If + * there is no non-null value, the function returns NULL. + */ + public OutType mode() { Review Comment: This adds a method to a `@PublicEvolving` class, so the public API answer in the description should be yes. ########## flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/aggregate/ModeAggFunction.java: ########## @@ -0,0 +1,175 @@ +/* + * 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.flink.table.runtime.functions.aggregate; + +import org.apache.flink.annotation.Internal; +import org.apache.flink.table.api.DataTypes; +import org.apache.flink.table.api.dataview.MapView; +import org.apache.flink.table.types.DataType; +import org.apache.flink.table.types.logical.LogicalType; + +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Objects; + +import static org.apache.flink.table.types.utils.DataTypeUtils.toInternalDataType; + +/** Built-in MODE with retraction aggregate function. */ +@Internal +public class ModeAggFunction<T> extends BuiltInAggregateFunction<T, ModeAggFunction.ModeAcc<T>> { + + private final transient DataType valueDataType; + + public ModeAggFunction(LogicalType valueType) { + this.valueDataType = toInternalDataType(valueType); + } + + @Override + public List<DataType> getArgumentDataTypes() { + return Collections.singletonList(valueDataType); + } + + @Override + public DataType getOutputDataType() { + return valueDataType; + } + + @Override + public T getValue(ModeAcc<T> accumulator) { + return accumulator.curMode; + } + + @Override + public ModeAcc<T> createAccumulator() { + final ModeAcc<T> acc = new ModeAcc<>(); + acc.buffer = new MapView<>(); + acc.curCnt = 0; + acc.curMode = null; + return acc; + } + + @Override + public DataType getAccumulatorDataType() { + return DataTypes.STRUCTURED( + ModeAcc.class, + DataTypes.FIELD("curCnt", DataTypes.BIGINT()), + DataTypes.FIELD("curMode", valueDataType.nullable()), + DataTypes.FIELD( + "buffer", + MapView.newMapViewDataType(valueDataType.notNull(), DataTypes.BIGINT()))); + } + + /** Accumulator for MODE. */ + public static class ModeAcc<T> { + public long curCnt; + public T curMode; + public MapView<T, Long> buffer; + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + ModeAcc<?> lagAcc = (ModeAcc<?>) o; + return curCnt == lagAcc.curCnt + && Objects.equals(curMode, lagAcc.curMode) + && Objects.equals(buffer, lagAcc.buffer); + } + + @Override + public int hashCode() { + return Objects.hash(curCnt, curMode, buffer); + } + } + + public void accumulate(ModeAcc<T> acc, T value) throws Exception { + if (value == null) { + return; + } + Long cnt = acc.buffer.get(value); + cnt = cnt == null ? 1 : cnt + 1; + acc.buffer.put(value, cnt); + if (Objects.equals(value, acc.curMode)) { + acc.curCnt = cnt; + } else if (cnt > acc.curCnt) { + acc.curMode = value; + acc.curCnt = cnt; + } + } + + public void retract(ModeAcc<T> acc, T value) throws Exception { + if (value == null) { + return; + } + Long cnt = acc.buffer.get(value); + cnt = cnt == null ? 0L : cnt - 1; + if (cnt <= 0) { + acc.buffer.remove(value); + } else { + acc.buffer.put(value, cnt); + } + acc.curCnt = cnt; + if (Objects.equals(value, acc.curMode)) { + for (Map.Entry<T, Long> entry : acc.buffer.entries()) { + if (entry.getValue() > acc.curCnt) { + acc.curMode = entry.getKey(); + acc.curCnt = entry.getValue(); + } + } + } + if (acc.curCnt == 0) { + acc.curMode = null; + } + } + + public void resetAccumulator(ModeAcc<T> acc) { + acc.buffer.clear(); + acc.curCnt = 0; + acc.curMode = null; + } + + public void merge(ModeAggFunction.ModeAcc<T> acc, Iterable<ModeAggFunction.ModeAcc<T>> its) + throws Exception { + for (ModeAggFunction.ModeAcc<T> otherAcc : its) { + if (!otherAcc.buffer.iterator().hasNext()) { + // otherAcc is empty, skip it + continue; + } + for (Map.Entry<T, Long> entry : otherAcc.buffer.entries()) { + final T key = entry.getKey(); + final long newValue; + if (acc.buffer.contains(key)) { + newValue = acc.buffer.get(key) + entry.getValue(); + acc.buffer.put(key, newValue); + } else { + newValue = entry.getValue(); + acc.buffer.put(key, newValue); + } + if (newValue > acc.curCnt) { Review Comment: With negative counts, merge can also lower the count of the current mode, so it needs the same rescan as retract. ########## flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/aggfunctions/ModeAggFunctionTest.java: ########## @@ -0,0 +1,76 @@ +/* + * 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.flink.table.planner.functions.aggfunctions; + +import org.apache.flink.table.data.StringData; +import org.apache.flink.table.runtime.functions.aggregate.ModeAggFunction; +import org.apache.flink.table.types.logical.VarCharType; + +import java.lang.reflect.Method; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +import static org.apache.flink.table.data.StringData.fromString; + +/** Test case for built-in MODE with retraction aggregate function. */ +final class ModeAggFunctionTest + extends AggFunctionTestBase<StringData, StringData, ModeAggFunction.ModeAcc<StringData>> { + + @Override + protected List<List<StringData>> getInputValueSets() { Review Comment: None of the tests send retractions through SQL. A `BuiltInAggregateFunctionTestBase` case with DELETE rows, plus one with mini-batch and `TWO_PHASE`, would cover both. -- 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]
