[GitHub] flink pull request #5174: [FLINK-8274][TableAPI & SQL] Fix Java 64K method c...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/flink/pull/5174


---

[GitHub] flink pull request #5174: [FLINK-8274][TableAPI & SQL] Fix Java 64K method c...

[jahandarm]

Github user jahandarm commented on a diff in the pull request:

https://github.com/apache/flink/pull/5174#discussion_r171410369
  
--- Diff: 
flink-libraries/flink-table/src/test/scala/org/apache/flink/table/runtime/stream/table/CalcWithSplitCodeGenITCase.scala
 ---
@@ -0,0 +1,385 @@
+/*
+ * 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.stream.table
+
+import org.apache.flink.api.scala._
+import org.apache.flink.streaming.api.scala.StreamExecutionEnvironment
+import org.apache.flink.streaming.util.StreamingMultipleProgramsTestBase
--- End diff --

Yes, you are right. I replaced it with "AbstractTestBase" on my copy of 
Flink source code.


---

[GitHub] flink pull request #5174: [FLINK-8274][TableAPI & SQL] Fix Java 64K method c...

[walterddr]

Github user walterddr commented on a diff in the pull request:

https://github.com/apache/flink/pull/5174#discussion_r171346253
  
--- Diff: 
flink-libraries/flink-table/src/test/scala/org/apache/flink/table/runtime/stream/table/CalcWithSplitCodeGenITCase.scala
 ---
@@ -0,0 +1,385 @@
+/*
+ * 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.stream.table
+
+import org.apache.flink.api.scala._
+import org.apache.flink.streaming.api.scala.StreamExecutionEnvironment
+import org.apache.flink.streaming.util.StreamingMultipleProgramsTestBase
--- End diff --

Yes, it does but not in flink-table module


---

[GitHub] flink pull request #5174: [FLINK-8274][TableAPI & SQL] Fix Java 64K method c...

[walterddr]

Github user walterddr commented on a diff in the pull request:

https://github.com/apache/flink/pull/5174#discussion_r170804955
  
--- Diff: 
flink-libraries/flink-table/src/main/scala/org/apache/flink/table/codegen/FunctionCodeGenerator.scala
 ---
@@ -41,7 +41,7 @@ import org.apache.flink.table.codegen.Indenter.toISC
 class FunctionCodeGenerator(
 config: TableConfig,
 nullableInput: Boolean,
-input1: TypeInformation[_ <: Any],
+val input1: TypeInformation[_ <: Any],
--- End diff --

Any specific reason for this change? typo?


---

[GitHub] flink pull request #5174: [FLINK-8274][TableAPI & SQL] Fix Java 64K method c...

[walterddr]

Github user walterddr commented on a diff in the pull request:

https://github.com/apache/flink/pull/5174#discussion_r171358583
  
--- Diff: 
flink-libraries/flink-table/src/main/scala/org/apache/flink/table/plan/nodes/CommonCalc.scala
 ---
@@ -46,10 +47,22 @@ trait CommonCalc {
   returnSchema.fieldNames,
   calcProjection)
 
+val (defines, bodies, callings) = generateCalcSplitFunctions(
+  generator,
+  projection.codeBuffer,
+  config.getMaxGeneratedCodeLength)
+
+val split = config.getMaxGeneratedCodeLength < projection.code.length
+val projectionCode = if (split && !projection.codeBuffer.isEmpty) {
--- End diff --

We can probably add this to the `generatedCalcSplitFunctions`, and let it 
always return the `samHeader` function (split or not), and with optional 
`defines` and `bodies`. This encapsulates all splitting functionality in one 
place, what do you think?


---

[GitHub] flink pull request #5174: [FLINK-8274][TableAPI & SQL] Fix Java 64K method c...

[walterddr]

Github user walterddr commented on a diff in the pull request:

https://github.com/apache/flink/pull/5174#discussion_r171360773
  
--- Diff: 
flink-libraries/flink-table/src/test/scala/org/apache/flink/table/runtime/stream/table/CalcWithSplitCodeGenITCase.scala
 ---
@@ -0,0 +1,385 @@
+/*
+ * 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.stream.table
+
+import org.apache.flink.api.scala._
+import org.apache.flink.streaming.api.scala.StreamExecutionEnvironment
+import org.apache.flink.streaming.util.StreamingMultipleProgramsTestBase
+import org.apache.flink.table.api.scala._
+import org.apache.flink.table.api.{TableConfig, TableEnvironment}
+import org.apache.flink.table.expressions.Literal
+import org.apache.flink.table.expressions.utils.{Func13, RichFunc1, 
RichFunc2}
+import org.apache.flink.table.runtime.utils.{StreamITCase, StreamTestData, 
UserDefinedFunctionTestUtils}
+import org.apache.flink.types.Row
+import org.junit.Assert.assertEquals
+import org.junit.Test
+
+import scala.collection.mutable
+
+/**
+  * set TableConfig's MaxGeneratedCodeLength to 1
+  * make sure every expression is in an independent function call
+  */
+class CalcWithSplitCodeGenITCase extends StreamingMultipleProgramsTestBase 
{
--- End diff --

I see you probably copied all tests in `CalcITCase` only to add the 
configuration for the max codegen length to 1. Why not try using something 
similar to `TableProgramsTestBase` to add two unique test configs? 


---

[GitHub] flink pull request #5174: [FLINK-8274][TableAPI & SQL] Fix Java 64K method c...

[walterddr]

Github user walterddr commented on a diff in the pull request:

https://github.com/apache/flink/pull/5174#discussion_r171357748
  
--- Diff: 
flink-libraries/flink-table/src/main/scala/org/apache/flink/table/plan/nodes/CommonCalc.scala
 ---
@@ -171,4 +186,47 @@ trait CommonCalc {
   rowCnt
 }
   }
+
+  /**
+* split origin generated code to split function calls, only used for 
calc.
+* @param generator
+* @param codeBuffer
+* @param maxLength
+* @return (method definitions, method bodies, method callings) of 
split function calls
+*/
+  private def generateCalcSplitFunctions(
--- End diff --

Seems like in order to avoid the out of compilation limit issue, you need 
to put this in all Common nodes. 
Any changes we can put it in `plan/util` and make it more generic? will 
probably imagine same thing could happen to any plan node that requires to 
generate a row of outputs.


---

[GitHub] flink pull request #5174: [FLINK-8274][TableAPI & SQL] Fix Java 64K method c...

[jahandarm]

Github user jahandarm commented on a diff in the pull request:

https://github.com/apache/flink/pull/5174#discussion_r168942867
  
--- Diff: 
flink-libraries/flink-table/src/test/scala/org/apache/flink/table/runtime/stream/table/CalcWithSplitCodeGenITCase.scala
 ---
@@ -0,0 +1,385 @@
+/*
+ * 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.stream.table
+
+import org.apache.flink.api.scala._
+import org.apache.flink.streaming.api.scala.StreamExecutionEnvironment
+import org.apache.flink.streaming.util.StreamingMultipleProgramsTestBase
--- End diff --

I think this class doesn't exist.


---

[GitHub] flink pull request #5174: [FLINK-8274][TableAPI & SQL] Fix Java 64K method c...

[Xpray]

GitHub user Xpray opened a pull request:

https://github.com/apache/flink/pull/5174

[FLINK-8274][TableAPI & SQL] Fix Java 64K method compiling limitation…

[FLINK-8274][TableAPI & SQL] Fix Java 64K method compiling limitation for 
CommonCalc'


## What is the purpose of the change

The genereted code in ```CommonCalc``` consists of Three parts, reusable 
code per code, ```filterCondition.code``` and ```projection.code```, 
```projection.code``` is most likely the longest parts, this issue intend to 
split ```projection.code``` to split function calls

## Brief change log

  - *add a configuration to ```TableConfig```, which indicates the max 
length of generated code*
  - *the default value for maxGenerateCodeLength is 48k*

## Verifying this change


This change is already covered by existing tests, such as:

all tests that involve with  the ```CommonCalc```

This change added tests and can be verified as follows:

  - *CalcWithSplitCodeGenITCase which set maxGenerateCodeLength to 1*

## Does this pull request potentially affect one of the following parts:

  - Dependencies (does it add or upgrade a dependency): 
 no
  - The public API, i.e., is any changed class annotated with 
`@Public(Evolving)`: 
no
  - The serializers: 
no
  - The runtime per-record code paths (performance sensitive): 
no
  - Anything that affects deployment or recovery: JobManager (and its 
components), Checkpointing, Yarn/Mesos, ZooKeeper: 
no
  - The S3 file system connector: 
no
## Documentation

  - Does this pull request introduce a new feature? 
no


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/Xpray/flink FLINK-8274

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/flink/pull/5174.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #5174


commit e8c697b284d708558debf6a790b817b2e30a1612
Author: Xpray 
Date:   2017-12-18T10:32:16Z

[FLINK-8274][TableAPI & SQL] Fix Java 64K method compiling limitation for 
CommonCalc




---