[GitHub] [flink] twalthr commented on a change in pull request #12232: [FLINK-15947] Finish moving scala expression DSL to flink-table-api-scala

[GitBox <gi...@apache.org>]

twalthr commented on a change in pull request #12232:
URL: https://github.com/apache/flink/pull/12232#discussion_r427791839



##########
File path: flink-table/flink-table-api-scala/src/main/scala/org/apache/flink/table/api/package.scala
##########
@@ -39,7 +37,7 @@ import org.apache.flink.table.api.{ImplicitExpressionConversions, ImplicitExpres
   *
   * Please refer to the website documentation about how to construct and run table programs.
   */
-package object api /* extends ImplicitExpressionConversions */ {
+package object api extends ImplicitExpressionConversions {
 
   // This package object should extend from ImplicitExpressionConversions but would clash with

Review comment:
       update comment

##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/expressions/package.scala
##########
@@ -21,8 +21,8 @@ package org.apache.flink.table
  * This package contains the base class of AST nodes and all the expression language AST classes.
  * Expression trees should not be manually constructed by users. They are implicitly constructed
  * from the implicit DSL conversions in
- * [[org.apache.flink.table.api.scala.ImplicitExpressionConversions]] and
- * [[org.apache.flink.table.api.scala.ImplicitExpressionOperations]]. For the Java API,
+ * [[org.apache.flink.table.api.bridge.scala.ImplicitExpressionConversions]] and
+ * [[org.apache.flink.table.api.bridge.scala.ImplicitExpressionOperations]]. For the Java API,

Review comment:
       update last sentence

##########
File path: flink-table/flink-table-api-scala/src/main/scala/org/apache/flink/table/api/expressionDsl.scala
##########
@@ -33,7 +33,7 @@ import java.sql.{Date, Time, Timestamp}
 import java.time.{LocalDate, LocalDateTime, LocalTime}
 import java.util.{List => JList, Map => JMap}

Review comment:
       nit: Shall we move `ImplicitExpressionConversions` and `ImplicitExpressionOperations` into dedicated files? Personally, I don't like `expressionDsl.scala` much. `TableConversions` and `DataStreamConvertions` are also dedicated files.

##########
File path: docs/dev/table/common.md
##########
@@ -552,7 +552,9 @@ val revenue = orders
 // execute query
 {% endhighlight %}
 
-**Note:** The Scala Table API uses Scala Symbols, which start with a single tick (`'`) to reference the attributes of a `Table`. The Table API uses Scala implicits. Make sure to import `org.apache.flink.api.scala._` and `org.apache.flink.table.api.scala._` in order to use Scala implicit conversions.
+**Note:** The Scala Table API uses Scala Symbols, which start with a single tick (`'`) to reference the attributes of a `Table`. The Table API uses Scala implicits. Make sure to import 

Review comment:
       `The Scala Table API uses Scala Symbols, which start with a single tick (`'`)`, update?

##########
File path: flink-scala-shell/src/main/scala/org/apache/flink/api/scala/FlinkILoop.scala
##########
@@ -23,7 +23,7 @@ import org.apache.flink.api.java.{JarHelper, ScalaShellEnvironment, ScalaShellSt
 import org.apache.flink.streaming.api.scala.StreamExecutionEnvironment
 import org.apache.flink.configuration.Configuration
 import org.apache.flink.table.api.EnvironmentSettings
-import org.apache.flink.table.api.scala.{BatchTableEnvironment, StreamTableEnvironment}
+import org.apache.flink.table.api.bridge.scala.{BatchTableEnvironment, StreamTableEnvironment}

Review comment:
       Update the default imports in the lower part of this class.

##########
File path: flink-table/flink-table-api-scala-bridge/src/main/scala/org/apache/flink/table/api/bridge/scala/package.scala
##########
@@ -50,7 +50,7 @@ import _root_.scala.language.implicitConversions
   * Please refer to the website documentation about how to construct and run table programs that are
   * connected to the DataStream API.
   */
-package object scala extends ImplicitExpressionConversions {
+package object scala {
 
   // This package object should not extend from ImplicitExpressionConversions but would clash with

Review comment:
       update comment




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org