Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add partial udf #3363

Open
wants to merge 16 commits into
base: feature/calcite-engine
Choose a base branch
from

Conversation

xinyual
Copy link

@xinyual xinyual commented Feb 27, 2025

Description

In this PR we add several UDFs and UDAFs. We add implement for functions which exist in both index PPL and Spark PPL from #3310.

  1. For those we can find corresponding function in calcite built-in functions, we map it and do some arguments modification if needed.
  2. For those we cannot find corresponding function in calcite built-in functions, we follow the design in [FEATURE] Calcite Engine Framework: General function (UDF/UDAF) framework #3310 to implement it.
  3. We add standalone UT.

Briefly, we implement all math/string/condition functions list in #3310 plus percentile and take aggregation.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

xinyual and others added 14 commits February 24, 2025 10:42
Signed-off-by: xinyual <xinyual@amazon.com>
Signed-off-by: xinyual <xinyual@amazon.com>
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
Signed-off-by: xinyual <xinyual@amazon.com>
Signed-off-by: Xinyuan Lu <xinyual@amazon.com>
Map opensearch math functions to calcite implementations
Signed-off-by: xinyual <xinyual@amazon.com>
Signed-off-by: xinyual <xinyual@amazon.com>
Signed-off-by: xinyual <xinyual@amazon.com>
Signed-off-by: xinyual <xinyual@amazon.com>
Signed-off-by: xinyual <xinyual@amazon.com>
Signed-off-by: xinyual <xinyual@amazon.com>
Signed-off-by: xinyual <xinyual@amazon.com>
Signed-off-by: xinyual <xinyual@amazon.com>
Signed-off-by: xinyual <xinyual@amazon.com>
@LantaoJin
Copy link
Member

LantaoJin commented Feb 27, 2025

  1. Rename the title to include what kind functions this PR added.
  2. Add CalcitePPLBuiltinFunctionTest extends CalcitePPLAbstractTest for UT (this is important to check the plan generated by Calcite, example in Implement ppl join command with Calcite #3364)
  3. Add more ITs refer the PPL query cases in https://github.com/opensearch-project/opensearch-spark/blob/main/integ-test/src/integration/scala/org/opensearch/flint/spark/ppl/FlintSparkPPLBuiltinFunctionITSuite.scala and other FlintSparkPPLXXFunctionITSuite

@@ -44,7 +44,7 @@ apply plugin: 'java'
apply plugin: 'io.freefair.lombok'
apply plugin: 'com.wiredforcode.spawn'

String baseVersion = "2.17.0"
String baseVersion = "2.17.1"
Copy link
Member

Choose a reason for hiding this comment

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

revert this change

@@ -5,9 +5,16 @@

package org.opensearch.sql.calcite.standalone;

import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_BANK;
import static org.opensearch.sql.legacy.TestsConstants.*;
Copy link
Member

Choose a reason for hiding this comment

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

Do not use * in imports. (disable merging imports in IDE)


@Ignore
@Test
public void testDate() {
Copy link
Member

Choose a reason for hiding this comment

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

Move all functions related tests to a new CalcitePPLBuiltinFunctionIT, this one is not for functions ITs.

@@ -0,0 +1,5 @@
package org.opensearch.sql.calcite.udf;
Copy link
Member

Choose a reason for hiding this comment

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

new file should include a copyright header

@LantaoJin LantaoJin added the calcite calcite migration releated label Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
calcite calcite migration releated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants