You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by twalthr <gi...@git.apache.org> on 2016/07/06 18:22:54 UTC
[GitHub] flink pull request #2209: [FLINK-4111] [table] Flink Table & SQL doesn't wor...
GitHub user twalthr opened a pull request:
https://github.com/apache/flink/pull/2209
[FLINK-4111] [table] Flink Table & SQL doesn't work in very simple example
Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide](http://flink.apache.org/how-to-contribute.html).
In addition to going through the list, please provide a meaningful description of your changes.
- [x] General
- The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
- The pull request addresses only one issue
- Each commit in the PR has a meaningful commit message (including the JIRA id)
- [x] Documentation
- Documentation has been added for new functionality
- Old documentation affected by the pull request has been updated
- JavaDoc for public methods has been added
- [x] Tests & Build
- Functionality added by the pull request is covered by tests
- `mvn clean verify` has been executed successfully locally or a Travis build has passed
This PR reworks the `flink-table` `pom.xml`. It simplifies the file, removes unnecessary dependencies and allows to build jobs with `flink-table` dependency.
@rmetzger @tillrohrmann Would be great if you could take a look on it.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/twalthr/flink FLINK-4111_2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2209.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 #2209
----
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2209: [FLINK-4111] [table] Flink Table & SQL doesn't work in ve...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the issue:
https://github.com/apache/flink/pull/2209
I think the change is good to merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2209: [FLINK-4111] [table] Flink Table & SQL doesn't wor...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/2209
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2209: [FLINK-4111] [table] Flink Table & SQL doesn't wor...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/2209#discussion_r69899109
--- Diff: flink-libraries/flink-table/pom.xml ---
@@ -126,14 +125,15 @@ under the License.
<plugin>
<groupId>net.alchim31.maven</groupId>
<artifactId>scala-maven-plugin</artifactId>
- <version>3.1.4</version>
+ <version>3.2.2</version>
--- End diff --
Why did you update the plugin version?
The problem is now that we are using different plugin versions across the project.
I would suggest to remove the plugin versions from all usages of the plugin and put it into the `pluginManagement` section of the root-pom.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2209: [FLINK-4111] [table] Flink Table & SQL doesn't work in ve...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/2209
Thanks @rmetzger. I would merge it today if there are no objections.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2209: [FLINK-4111] [table] Flink Table & SQL doesn't wor...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/2209#discussion_r69900273
--- Diff: flink-libraries/flink-table/pom.xml ---
@@ -148,80 +148,22 @@ under the License.
</goals>
</execution>
</executions>
- <configuration>
- <jvmArgs>
- <jvmArg>-Xms128m</jvmArg>
- <jvmArg>-Xmx512m</jvmArg>
- </jvmArgs>
- <compilerPlugins combine.children="append">
- <compilerPlugin>
- <groupId>org.scalamacros</groupId>
- <artifactId>paradise_${scala.version}</artifactId>
- <version>${scala.macros.version}</version>
- </compilerPlugin>
- </compilerPlugins>
- </configuration>
</plugin>
- <!-- Eclipse Integration -->
<plugin>
<groupId>org.apache.maven.plugins</groupId>
- <artifactId>maven-eclipse-plugin</artifactId>
- <version>2.8</version>
- <configuration>
- <downloadSources>true</downloadSources>
- <projectnatures>
- <projectnature>org.scala-ide.sdt.core.scalanature</projectnature>
- <projectnature>org.eclipse.jdt.core.javanature</projectnature>
- </projectnatures>
- <buildcommands>
- <buildcommand>org.scala-ide.sdt.core.scalabuilder</buildcommand>
- </buildcommands>
- <classpathContainers>
- <classpathContainer>org.scala-ide.sdt.launching.SCALA_CONTAINER</classpathContainer>
- <classpathContainer>org.eclipse.jdt.launching.JRE_CONTAINER</classpathContainer>
- </classpathContainers>
- <excludes>
- <exclude>org.scala-lang:scala-library</exclude>
- <exclude>org.scala-lang:scala-compiler</exclude>
- </excludes>
- <sourceIncludes>
- <sourceInclude>**/*.scala</sourceInclude>
- <sourceInclude>**/*.java</sourceInclude>
- </sourceIncludes>
- </configuration>
- </plugin>
--- End diff --
Did you check that flink-table still works in eclipse without this plugin?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2209: [FLINK-4111] [table] Flink Table & SQL doesn't work in ve...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/2209
Merging...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2209: [FLINK-4111] [table] Flink Table & SQL doesn't wor...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/2209#discussion_r69900113
--- Diff: flink-libraries/flink-table/pom.xml ---
@@ -148,80 +148,22 @@ under the License.
</goals>
</execution>
</executions>
- <configuration>
- <jvmArgs>
- <jvmArg>-Xms128m</jvmArg>
- <jvmArg>-Xmx512m</jvmArg>
- </jvmArgs>
- <compilerPlugins combine.children="append">
- <compilerPlugin>
- <groupId>org.scalamacros</groupId>
- <artifactId>paradise_${scala.version}</artifactId>
- <version>${scala.macros.version}</version>
- </compilerPlugin>
--- End diff --
Are the macros only needed in flink-scala? I thought they are required for the type extraction using macros?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2209: [FLINK-4111] [table] Flink Table & SQL doesn't wor...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/2209#discussion_r70038156
--- Diff: flink-libraries/flink-table/pom.xml ---
@@ -148,80 +148,22 @@ under the License.
</goals>
</execution>
</executions>
- <configuration>
- <jvmArgs>
- <jvmArg>-Xms128m</jvmArg>
- <jvmArg>-Xmx512m</jvmArg>
- </jvmArgs>
- <compilerPlugins combine.children="append">
- <compilerPlugin>
- <groupId>org.scalamacros</groupId>
- <artifactId>paradise_${scala.version}</artifactId>
- <version>${scala.macros.version}</version>
- </compilerPlugin>
--- End diff --
Most of the dependencies I removed here are defined in `flink-scala`. Other Scala projects like `flink-cep-scala` or `flink-gelly-scala` also do not have this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2209: [FLINK-4111] [table] Flink Table & SQL doesn't work in ve...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the issue:
https://github.com/apache/flink/pull/2209
Are all the dependencies you removed really not needed or are they needed, but present because they are pulled in transitively?
In general, it makes sense to define all needed dependencies, instead of relying on transitive dependencies.
The problem with the suggested shading is that you are not relocating calcite, which means that we have calcite in the org.apache.calcite namespace, in our flink package.
This can potentially lead to version clashes if our users want to use a different calcite version in their code (I have to admit that's unlikely.)
But for `com.fasterxml.jackson` this might be an issue, also for `org.slf4j`.
I would exclude slf4j and jackson from the shading.
Do you know why the relocation of calcite doesn't work? Do you think there is an issue with relocating class files produced by the scala compiler?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2209: [FLINK-4111] [table] Flink Table & SQL doesn't work in ve...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/2209
@rmetzger I have reworked the `pom.xml` again. Now the JAR is as clean as possible. All unneccessary files are filtered out. Most of the other dependencies (`com.google`, `com.fasterxml.jackson`) are relocated.
I could not relocate `org.slf4j` because that would lead to a warning during runtime. Relocating Calcite leads to weird Scala compiler problems, but I think no one wants to use a custom Caclite version.
I have introduced a global scala maven plugin version for all scala modules.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2209: [FLINK-4111] [table] Flink Table & SQL doesn't wor...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/2209#discussion_r70034510
--- Diff: flink-libraries/flink-table/pom.xml ---
@@ -126,14 +125,15 @@ under the License.
<plugin>
<groupId>net.alchim31.maven</groupId>
<artifactId>scala-maven-plugin</artifactId>
- <version>3.1.4</version>
+ <version>3.2.2</version>
--- End diff --
I used the newest library `flink-cep-scala` as reference. We are already using different plugin version. I will introduce a global variable for all projects.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2209: [FLINK-4111] [table] Flink Table & SQL doesn't wor...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/2209#discussion_r69900431
--- Diff: flink-libraries/flink-table/pom.xml ---
@@ -239,23 +181,4 @@ under the License.
</plugins>
</build>
- <profiles>
- <profile>
- <id>scala-2.10</id>
- <activation>
- <property>
- <!-- this is the default scala profile -->
- <name>!scala-2.11</name>
- </property>
- </activation>
- <dependencies>
- <dependency>
- <groupId>org.scalamacros</groupId>
- <artifactId>quasiquotes_${scala.binary.version}</artifactId>
--- End diff --
Isn't this module using quasiquotes?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---