[hotfix] ELEMENT should throw TableRuntimeException

[nateab]

RuntimeException is too generic, we should throw a more specific TableRuntimeException here.

[flinkbot]

CI report:

• f5fb7bb Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

• @flinkbot run azure re-run the last Azure build

[davidradl]

looking in the other generate functions in this class they throw CodeGenException or ValidationException. No other function in this class throws a TableRuntimeException or a RuntimeException. I wonder if it is right for this code to throw a RuntimeException or the Table version of it.

Also please add a unit test to check the new Exception type

[nateab]

@twalthr was the one who suggested this should be a TableRuntimeException , but I agree that perhaps ValidationException might be a better fit, what do you think Timo?

Will add a test as well thanks

[pgaref]

Looks like the ArrayElement builtIn function is specific to TableApi but I agree it would make sense to align with the rest and throw a ValidationException

[snuyanzin]

Ideally ValidationException should be used if something failed during validation phase
TableRuntimeException should be used if validation passed and error happened in runtime

[nateab]

We should use TableRuntimeException then I believe, since we don't know the actual size of the array until runtime.

I tried adding a test, but it's not straightforward to me how to test this runtime exception without breaking the testing framework

+++ b/flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/expressions/ArrayTypeTest.scala
@@ -21,7 +21,6 @@ import org.apache.flink.core.testutils.FlinkAssertions.anyCauseMatches
 import org.apache.flink.table.api._
 import org.apache.flink.table.planner.expressions.utils.ArrayTypeTestBase
 import org.apache.flink.table.planner.utils.DateTimeTestUtil.{localDate, localDateTime, localTime => gLocalTime}
-
 import org.assertj.core.api.Assertions.assertThatThrownBy
 import org.junit.jupiter.api.Test

@@ -242,6 +241,13 @@ class ArrayTypeTest extends ArrayTypeTestBase {
       "Array element access needs an index starting at 1 but was 0.")
   }

+  @Test
+  def testElement(): Unit = {
+    testExpectedSqlException(
+      "ELEMENT(f2)",
+      "Array has more than one element.")
+  }
+
   @Test
   def testReturnNullWhenArrayIndexOutOfBounds(): Unit = {
     // ARRAY<INT NOT NULL>
@@ -250,4 +256,11 @@ class ArrayTypeTest extends ArrayTypeTestBase {
     // ARRAY<INT>
     testAllApis('f11.at(3), "f11[4]", "NULL")
   }
+
+  @Test
+  def testElementFailsOnMultiElementArray(): Unit = {
+    testExpectedSqlException(
+      "ELEMENT(ARRAY[1, 2])",
+      "Array has more than one element.")
+  }
 }

These tests would fail with something like:

java.lang.AssertionError: 
Expecting code to raise a throwable.

	at org.apache.flink.table.planner.expressions.ArrayTypeTest.testElementFailsOnMultiElementArrayInTableApi(ArrayTypeTest.scala:271)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.concurrent.ForkJoinTask.doExec$$$capture(ForkJoinTask.java:373)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1182)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1655)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1622)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:165)
	Suppressed: java.lang.AssertionError: Error when executing the expression. Expression code:

      
      // Using option 'table.exec.legacy-cast-behaviour':'false'
      // Timezone: org.apache.flink.table.api.TableConfig@11f6af40
          
      public class TestFunction$5
          extends org.apache.flink.api.common.functions.RichMapFunction {

        
        org.apache.flink.table.data.binary.BinaryArrayData array$0 = new org.apache.flink.table.data.binary.BinaryArrayData();
        org.apache.flink.table.data.writer.BinaryArrayWriter writer$1 = new org.apache.flink.table.data.writer.BinaryArrayWriter(array$0, 2, 4);
               
        org.apache.flink.table.data.binary.BinaryRowData out = new org.apache.flink.table.data.binary.BinaryRowData(1);
        org.apache.flink.table.data.writer.BinaryRowWriter outWriter = new org.apache.flink.table.data.writer.BinaryRowWriter(out);

        public TestFunction$5(Object[] references) throws Exception {
          
          writer$1.reset();
          
          
          if (false) {
            writer$1.setNullInt(0);
          } else {
            writer$1.writeInt(0, ((int) 1));
          }
                    
          
          
          if (false) {
            writer$1.setNullInt(1);
          } else {
            writer$1.writeInt(1, ((int) 2));
          }
                    
          writer$1.complete();
                   
        }

        

        @Override
        public void open(org.apache.flink.api.common.functions.OpenContext openContext) throws Exception {
          
        }

        @Override
        public Object map(Object _in1) throws Exception {
          org.apache.flink.table.data.RowData in1 = (org.apache.flink.table.data.RowData) _in1;
          
          boolean isNull$3;
          org.apache.flink.table.data.binary.BinaryStringData result$4;
          
          
          
          
          outWriter.reset();
          
          
          
          boolean isNull$2;
          int result$2;
          switch (false ? 0 : array$0.size()) {
            case 0:
              isNull$2 = true;
              result$2 = -1;
              break;
            case 1:
              isNull$2 = array$0.isNullAt(0);
              result$2 = isNull$2 ? -1 : array$0.getInt(0);
              break;
            default:
              throw new org.apache.flink.table.api.ValidationException("Array has more than one element.");
          }
          
           // --- Cast section generated by org.apache.flink.table.planner.functions.casting.CharVarCharTrimPadCastRule
           isNull$3 = isNull$2;
          if (!isNull$3) {
          result$4 = org.apache.flink.table.data.binary.BinaryStringData.fromString("" + result$2);
          isNull$3 = result$4 == null;
          } else {
          result$4 = org.apache.flink.table.data.binary.BinaryStringData.EMPTY_UTF8;
          }
          
           // --- End cast section
                         
          if (isNull$3) {
            outWriter.setNullAt(0);
          } else {
            outWriter.writeString(0, result$4);
          }
                     
          outWriter.complete();
                  
          return out;
                  
        }

        @Override
        public void close() throws Exception {
          
        }

        
      }
    
		at org.apache.flink.table.planner.expressions.utils.ExpressionTestBase.evaluateFunctionResult(ExpressionTestBase.scala:285)
		at org.apache.flink.table.planner.expressions.utils.ExpressionTestBase.evaluateGivenExprs(ExpressionTestBase.scala:346)
		at org.apache.flink.table.planner.expressions.utils.ExpressionTestBase.evaluateExprs(ExpressionTestBase.scala:141)
		... 7 more
	Caused by: org.apache.flink.table.api.ValidationException: Array has more than one element.
		at TestFunction$5.map(Unknown Source)
		at org.apache.flink.table.planner.expressions.utils.ExpressionTestBase.evaluateFunctionResult(ExpressionTestBase.scala:260)
		... 9 more

Since existing tests pass with this change, could we skip adding a test since it is a small change?

[snuyanzin]

this is a legacy way of doing tests which is a way more limited ...

you can use a more modern way, for instance in CollectionFunctionsITCase add a test for this function and among others have something like

 .testSqlRuntimeError("element(ARRAY[1, 2])", TableRuntimeException.class, "Array has more than one element.")

this will test the change

[nateab]

Awesome thanks for the pointer Sergey! I've added a couple tests for ELEMENT there

[snuyanzin]

The ci issue seems to be related to the changes
@nateab could you please take a look
https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=68516&view=logs&j=0c940707-2659-5648-cbe6-a1ad63045f0a&t=019d96ac-af5c-5c6d-106d-b67f63cc6e96

[nateab]

@snuyanzin thanks for checking, I fixed the issue locally but hadn't pushed it yet. I needed to use the fully qualified class name of the new exception types.

[snuyanzin]

can we have a test for this?

i guess something like

SELECT element(array[1, 2, 3]);

[nateab]

Idk if you saw my comment here, but when I tried adding a test like that, it would break the testing framework

[snuyanzin]

commented