Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions core/src/main/java/org/opensearch/sql/calcite/udf/Accumulator.java

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,34 @@

package org.opensearch.sql.calcite.udf;

public interface UserDefinedAggFunction<S extends Accumulator> {
public interface UserDefinedAggFunction<S extends UserDefinedAggFunction.Accumulator> {
/**
* @return {@link Accumulator}
*/
S init();

/**
*
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format issue

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

* @param {@link Accumulator}
* @return final result
*/
Object result(S accumulator);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in UDF, the method name is eval, should we change to eval either?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the function name of agg function, it's defined in calcite here https://github.com/apache/calcite/blob/1793ba79a328c61fb42842f443334cc1353c985f/core/src/main/java/org/apache/calcite/schema/impl/AggregateFunctionImpl.java#L91. We cannot modify them. I will left comment.


// Add values to the accumulator
/**
* Add values to the accumulator. Notice some init argument will also be here like the 50 in Percentile(field, 50).
* @param acc {@link Accumulator}
* @param values the value to add to accumulator
* @return {@link Accumulator}
*/
S add(S acc, Object... values);

/**
* Maintain the state when {@link UserDefinedAggFunction} add values
*/
interface Accumulator {
/**
* @return the final aggregation value
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you run ./gradlew spotlessApply before pushing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forget to run. Just rerun the push.

*/
Object value();
}
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.sql.calcite.udf.udaf;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copyright header missing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


import java.util.ArrayList;
import java.util.List;
import org.opensearch.sql.calcite.udf.Accumulator;

import org.opensearch.sql.calcite.udf.UserDefinedAggFunction;

public class TakeAggFunction implements UserDefinedAggFunction<TakeAggFunction.TakeAccumulator> {
Expand All @@ -14,7 +19,7 @@ public TakeAccumulator init() {

@Override
public Object result(TakeAccumulator accumulator) {
return accumulator.result();
return accumulator.value();
}

@Override
Expand All @@ -40,7 +45,7 @@ public TakeAccumulator() {
}

@Override
public Object result() {
public Object value() {
return hits;
}
Comment on lines 47 to 49
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code indentation problem, please run ./gradlew spotlessApply

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,16 @@ static SqlOperator translate(String op) {
}
}

/**
* Translates function arguments to align with Calcite's expectations, ensuring compatibility
* with PPL (Piped Processing Language). This is necessary because Calcite's input argument
* order or default values may differ from PPL's function definitions.
*
* @param op The function name as a string.
* @param argList A list of {@link RexNode} representing the parsed arguments from the PPL statement.
* @param context The {@link CalcitePlanContext} providing necessary utilities such as {@code rexBuilder}.
* @return A modified list of {@link RexNode} that correctly maps to Calcite’s function expectations.
*/
static List<RexNode> translateArgument(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a framework work, we will change this method frequently, could you add some comments to explain this method for developers

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, already added.

String op, List<RexNode> argList, CalcitePlanContext context) {
switch (op.toUpperCase(Locale.ROOT)) {
Expand Down
Loading