You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/06/29 08:37:45 UTC
[GitHub] [beam] purbanow opened a new pull request #12117: [BEAM-10343] Add dispositions for SnowflakeIO.write
purbanow opened a new pull request #12117:
URL: https://github.com/apache/beam/pull/12117
This PR is part of adding SnowflakeIO to Java SDK [BEAM-9893](https://issues.apache.org/jira/browse/BEAM-9893). Precisely this PR is adding dispositions for write operation [BEAM-10343](https://issues.apache.org/jira/browse/BEAM-10343). Precisely following dispositions:
- CREATE_IF_NEEDED - default behaviour. The write operation checks whether the specified target table exists; if it does not, the write operation attempts to create the table Specify the schema for the target table using the .withTableSchema() option.
- CREATE_NEVER - The write operation fails if the target table does not exist.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [X] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [X] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [X] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | --- | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
----------------------------------------------------------------
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
[GitHub] [beam] purbanow commented on a change in pull request #12117: [BEAM-10343] Add dispositions for SnowflakeIO.write
Posted by GitBox <gi...@apache.org>.
purbanow commented on a change in pull request #12117:
URL: https://github.com/apache/beam/pull/12117#discussion_r451973374
##########
File path: sdks/java/io/snowflake/src/main/java/org/apache/beam/sdk/io/snowflake/services/SnowflakeServiceImpl.java
##########
@@ -25,7 +27,9 @@
import java.util.function.Consumer;
import java.util.stream.Collectors;
import javax.sql.DataSource;
+import org.apache.beam.sdk.io.snowflake.data.SnowflakeTableSchema;
import org.apache.beam.sdk.io.snowflake.enums.CloudProvider;
Review comment:
I see your point. I even removed this enum and add following variable to the Service file:
`private static final String GCS_PREFIX = "gs://";`
----------------------------------------------------------------
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
[GitHub] [beam] purbanow commented on pull request #12117: [BEAM-10343] Add dispositions for SnowflakeIO.write
Posted by GitBox <gi...@apache.org>.
purbanow commented on pull request #12117:
URL: https://github.com/apache/beam/pull/12117#issuecomment-652344088
@lgajowy will you find a moment to make a CR for this PR?
----------------------------------------------------------------
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
[GitHub] [beam] lgajowy merged pull request #12117: [BEAM-10343] Add dispositions for SnowflakeIO.write
Posted by GitBox <gi...@apache.org>.
lgajowy merged pull request #12117:
URL: https://github.com/apache/beam/pull/12117
----------------------------------------------------------------
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
[GitHub] [beam] lgajowy commented on pull request #12117: [BEAM-10343] Add dispositions for SnowflakeIO.write
Posted by GitBox <gi...@apache.org>.
lgajowy commented on pull request #12117:
URL: https://github.com/apache/beam/pull/12117#issuecomment-652925396
@purbanow I will try to take a look at that today or tomorrow. If I won't leave a review by then, please try to ping somebody else (that will mean that I did not have enough time to go through all this. :)
Something that draws my attention when I scanned the code quickly (bear in mind that I might be missing some Snowflake details): why do we need to create all the SnowflakeDataTypes? Is there a way to create them in one class (without specifying all the separate types for that)? That would (probably?) make things more compact.
----------------------------------------------------------------
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
[GitHub] [beam] purbanow commented on a change in pull request #12117: [BEAM-10343] Add dispositions for SnowflakeIO.write
Posted by GitBox <gi...@apache.org>.
purbanow commented on a change in pull request #12117:
URL: https://github.com/apache/beam/pull/12117#discussion_r450011282
##########
File path: sdks/java/io/snowflake/src/main/java/org/apache/beam/sdk/io/snowflake/SnowflakeIO.java
##########
@@ -155,7 +157,7 @@
* items.apply(
* SnowflakeIO.<KV<Integer, String>>write()
* .withDataSourceConfiguration(dataSourceConfiguration)
- * .withTable(table)
+ * .toTable(table)
Review comment:
Sure :)
----------------------------------------------------------------
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
[GitHub] [beam] purbanow commented on a change in pull request #12117: [BEAM-10343] Add dispositions for SnowflakeIO.write
Posted by GitBox <gi...@apache.org>.
purbanow commented on a change in pull request #12117:
URL: https://github.com/apache/beam/pull/12117#discussion_r450015267
##########
File path: sdks/java/io/snowflake/src/main/java/org/apache/beam/sdk/io/snowflake/services/SnowflakeServiceImpl.java
##########
@@ -25,7 +27,9 @@
import java.util.function.Consumer;
import java.util.stream.Collectors;
import javax.sql.DataSource;
+import org.apache.beam.sdk.io.snowflake.data.SnowflakeTableSchema;
import org.apache.beam.sdk.io.snowflake.enums.CloudProvider;
Review comment:
For now, I would keep it outside of serviceImpl as its easier to see which cloud providers currently are supported by IO.
----------------------------------------------------------------
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
[GitHub] [beam] purbanow commented on a change in pull request #12117: [BEAM-10343] Add dispositions for SnowflakeIO.write
Posted by GitBox <gi...@apache.org>.
purbanow commented on a change in pull request #12117:
URL: https://github.com/apache/beam/pull/12117#discussion_r450012687
##########
File path: sdks/java/io/snowflake/src/main/java/org/apache/beam/sdk/io/snowflake/services/SnowflakeServiceImpl.java
##########
@@ -169,6 +191,54 @@ private void prepareTableAccordingWriteDisposition(
}
}
+ private void createTableIfNotExists(
+ DataSource dataSource, String table, SnowflakeTableSchema tableSchema) throws SQLException {
+ String query =
+ String.format(
+ "SELECT EXISTS (SELECT 1 FROM information_schema.tables WHERE table_name = '%s');",
Review comment:
Totally not. Thanks for spotting this .
----------------------------------------------------------------
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
[GitHub] [beam] purbanow commented on a change in pull request #12117: [BEAM-10343] Add dispositions for SnowflakeIO.write
Posted by GitBox <gi...@apache.org>.
purbanow commented on a change in pull request #12117:
URL: https://github.com/apache/beam/pull/12117#discussion_r450218823
##########
File path: sdks/java/io/snowflake/src/test/java/org/apache/beam/sdk/io/snowflake/test/unit/data/SnowflakeBooleanTest.java
##########
@@ -0,0 +1,32 @@
+/*
+ * 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.beam.sdk.io.snowflake.test.unit.data;
+
+import static org.junit.Assert.assertEquals;
+
+import org.apache.beam.sdk.io.snowflake.data.logical.SnowflakeBoolean;
+import org.junit.Test;
+
+public class SnowflakeBooleanTest {
Review comment:
Thanks for this idea. I created parametrized test.
https://github.com/apache/beam/blob/b68646ffab38774864e421c6d4a704af5fd6a668/sdks/java/io/snowflake/src/test/java/org/apache/beam/sdk/io/snowflake/test/unit/data/SnowflakeDataTypeValidTest.java
----------------------------------------------------------------
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
[GitHub] [beam] purbanow edited a comment on pull request #12117: [BEAM-10343] Add dispositions for SnowflakeIO.write
Posted by GitBox <gi...@apache.org>.
purbanow edited a comment on pull request #12117:
URL: https://github.com/apache/beam/pull/12117#issuecomment-654253476
----------------------------------------------------------------
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
[GitHub] [beam] purbanow commented on pull request #12117: [BEAM-10343] Add dispositions for SnowflakeIO.write
Posted by GitBox <gi...@apache.org>.
purbanow commented on pull request #12117:
URL: https://github.com/apache/beam/pull/12117#issuecomment-654253476
@lgajowy thanks a lot for CR. I respond for your all comments. Will you find moment to check again?
----------------------------------------------------------------
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
[GitHub] [beam] lgajowy commented on a change in pull request #12117: [BEAM-10343] Add dispositions for SnowflakeIO.write
Posted by GitBox <gi...@apache.org>.
lgajowy commented on a change in pull request #12117:
URL: https://github.com/apache/beam/pull/12117#discussion_r451677939
##########
File path: sdks/java/io/snowflake/src/main/java/org/apache/beam/sdk/io/snowflake/services/SnowflakeServiceImpl.java
##########
@@ -25,7 +27,9 @@
import java.util.function.Consumer;
import java.util.stream.Collectors;
import javax.sql.DataSource;
+import org.apache.beam.sdk.io.snowflake.data.SnowflakeTableSchema;
import org.apache.beam.sdk.io.snowflake.enums.CloudProvider;
Review comment:
I'm not sure, to be honest. Users should look for that information in the docs. Note that technically (not that I'd wish for this) there are ways of providing support to other cloud providers without extending that enum so looking at the endpoint is not the ultimate proof that only gcs is supported. If there's no docs or it's out of date regarding cloud providers support, the devs will still need to dig into the IO implementation details to really find out. So having this enum public is not much help imho.
On the other hand, what we do now is that we leave an implementation detail (noise) that is not useful for the users in any way in code in the public API. I'd suggest hiding it and maybe expose it later if there is a good reason to do so. :)
----------------------------------------------------------------
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
[GitHub] [beam] lgajowy commented on a change in pull request #12117: [BEAM-10343] Add dispositions for SnowflakeIO.write
Posted by GitBox <gi...@apache.org>.
lgajowy commented on a change in pull request #12117:
URL: https://github.com/apache/beam/pull/12117#discussion_r449152583
##########
File path: sdks/java/io/snowflake/src/main/java/org/apache/beam/sdk/io/snowflake/SnowflakeIO.java
##########
@@ -155,7 +157,7 @@
* items.apply(
* SnowflakeIO.<KV<Integer, String>>write()
* .withDataSourceConfiguration(dataSourceConfiguration)
- * .withTable(table)
+ * .toTable(table)
Review comment:
nit: could you move this `toTable()` call below all `with...` calls?
##########
File path: sdks/java/io/snowflake/src/main/java/org/apache/beam/sdk/io/snowflake/SnowflakeIO.java
##########
@@ -593,7 +606,7 @@ public void populateDisplayData(DisplayData.Builder builder) {
*
* @param table - String with the name of the table.
*/
- public Write<T> withTable(String table) {
+ public Write<T> toTable(String table) {
Review comment:
Rename `toTable()` method to `to()`? Seems more consistent with other ios, eg. BigQueryIO and you already specify `String table` argument and the docs tells what's the subject of this operation so it's all clear. :)
##########
File path: sdks/java/io/snowflake/src/main/java/org/apache/beam/sdk/io/snowflake/services/SnowflakeServiceImpl.java
##########
@@ -169,6 +191,54 @@ private void prepareTableAccordingWriteDisposition(
}
}
+ private void createTableIfNotExists(
+ DataSource dataSource, String table, SnowflakeTableSchema tableSchema) throws SQLException {
+ String query =
+ String.format(
+ "SELECT EXISTS (SELECT 1 FROM information_schema.tables WHERE table_name = '%s');",
Review comment:
Are the double spaces needed here?
```suggestion
"SELECT EXISTS (SELECT 1 FROM information_schema.tables WHERE table_name = '%s');",
```
##########
File path: sdks/java/io/snowflake/src/main/java/org/apache/beam/sdk/io/snowflake/data/text/SnowflakeVarchar.java
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.beam.sdk.io.snowflake.data.text;
+
+import java.io.Serializable;
+import org.apache.beam.sdk.io.snowflake.data.SnowflakeDataType;
+
+public class SnowflakeVarchar implements SnowflakeDataType, Serializable {
+ private static final Long MAX_LENGTH = 16777216L;
+ private Long length;
+
+ public static SnowflakeVarchar of() {
+ return new SnowflakeVarchar();
+ }
+
+ public static SnowflakeVarchar of(long length) {
+ return new SnowflakeVarchar(length);
+ }
+
+ public SnowflakeVarchar() {}
+
+ public SnowflakeVarchar(long length) {
+ if (length > MAX_LENGTH) {
+ throw new IllegalArgumentException();
Review comment:
(Same as above)
##########
File path: sdks/java/io/snowflake/src/test/java/org/apache/beam/sdk/io/snowflake/test/unit/data/SnowflakeBooleanTest.java
##########
@@ -0,0 +1,32 @@
+/*
+ * 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.beam.sdk.io.snowflake.test.unit.data;
+
+import static org.junit.Assert.assertEquals;
+
+import org.apache.beam.sdk.io.snowflake.data.logical.SnowflakeBoolean;
+import org.junit.Test;
+
+public class SnowflakeBooleanTest {
Review comment:
I wonder if there's a way of testing all the Snowflake data types in one test/one testing `SnowflakeDataTypeTest` class (parametrized test?). :thinking: Worth trying imho but I didn't check this.
##########
File path: sdks/java/io/snowflake/src/main/java/org/apache/beam/sdk/io/snowflake/data/text/SnowflakeBinary.java
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.beam.sdk.io.snowflake.data.text;
+
+import java.io.Serializable;
+import org.apache.beam.sdk.io.snowflake.data.SnowflakeDataType;
+
+public class SnowflakeBinary implements SnowflakeDataType, Serializable {
+
+ private static final Long MAX_SIZE = 8388608L;
+
+ private Long size; // bytes
+
+ public SnowflakeBinary() {}
+
+ public static SnowflakeBinary of() {
+ return new SnowflakeBinary();
+ }
+
+ public static SnowflakeBinary of(long size) {
+ return new SnowflakeBinary(size);
+ }
+
+ public SnowflakeBinary(long size) {
+ if (size > MAX_SIZE) {
+ throw new IllegalArgumentException();
Review comment:
Does it make sense to add a message for the exception? Something like "Snowflake binary maximum size exceeded.".
##########
File path: sdks/java/io/snowflake/src/main/java/org/apache/beam/sdk/io/snowflake/services/SnowflakeServiceImpl.java
##########
@@ -25,7 +27,9 @@
import java.util.function.Consumer;
import java.util.stream.Collectors;
import javax.sql.DataSource;
+import org.apache.beam.sdk.io.snowflake.data.SnowflakeTableSchema;
import org.apache.beam.sdk.io.snowflake.enums.CloudProvider;
Review comment:
`CloudProvider` is used only in SnowflakeServiceImpl. I think it makes sense to keep it inside the serviceImpl as a private enum, wdyt?
----------------------------------------------------------------
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
[GitHub] [beam] lgajowy commented on pull request #12117: [BEAM-10343] Add dispositions for SnowflakeIO.write
Posted by GitBox <gi...@apache.org>.
lgajowy commented on pull request #12117:
URL: https://github.com/apache/beam/pull/12117#issuecomment-653116162
Run Java PreCommit
----------------------------------------------------------------
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
[GitHub] [beam] purbanow commented on a change in pull request #12117: [BEAM-10343] Add dispositions for SnowflakeIO.write
Posted by GitBox <gi...@apache.org>.
purbanow commented on a change in pull request #12117:
URL: https://github.com/apache/beam/pull/12117#discussion_r450018838
##########
File path: sdks/java/io/snowflake/src/main/java/org/apache/beam/sdk/io/snowflake/data/text/SnowflakeVarchar.java
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.beam.sdk.io.snowflake.data.text;
+
+import java.io.Serializable;
+import org.apache.beam.sdk.io.snowflake.data.SnowflakeDataType;
+
+public class SnowflakeVarchar implements SnowflakeDataType, Serializable {
+ private static final Long MAX_LENGTH = 16777216L;
+ private Long length;
+
+ public static SnowflakeVarchar of() {
+ return new SnowflakeVarchar();
+ }
+
+ public static SnowflakeVarchar of(long length) {
+ return new SnowflakeVarchar(length);
+ }
+
+ public SnowflakeVarchar() {}
+
+ public SnowflakeVarchar(long length) {
+ if (length > MAX_LENGTH) {
+ throw new IllegalArgumentException();
Review comment:
I added message. Thanks
----------------------------------------------------------------
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
[GitHub] [beam] purbanow commented on a change in pull request #12117: [BEAM-10343] Add dispositions for SnowflakeIO.write
Posted by GitBox <gi...@apache.org>.
purbanow commented on a change in pull request #12117:
URL: https://github.com/apache/beam/pull/12117#discussion_r450018715
##########
File path: sdks/java/io/snowflake/src/main/java/org/apache/beam/sdk/io/snowflake/data/text/SnowflakeBinary.java
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.beam.sdk.io.snowflake.data.text;
+
+import java.io.Serializable;
+import org.apache.beam.sdk.io.snowflake.data.SnowflakeDataType;
+
+public class SnowflakeBinary implements SnowflakeDataType, Serializable {
+
+ private static final Long MAX_SIZE = 8388608L;
+
+ private Long size; // bytes
+
+ public SnowflakeBinary() {}
+
+ public static SnowflakeBinary of() {
+ return new SnowflakeBinary();
+ }
+
+ public static SnowflakeBinary of(long size) {
+ return new SnowflakeBinary(size);
+ }
+
+ public SnowflakeBinary(long size) {
+ if (size > MAX_SIZE) {
+ throw new IllegalArgumentException();
Review comment:
I added message. Thanks
----------------------------------------------------------------
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
[GitHub] [beam] purbanow edited a comment on pull request #12117: [BEAM-10343] Add dispositions for SnowflakeIO.write
Posted by GitBox <gi...@apache.org>.
purbanow edited a comment on pull request #12117:
URL: https://github.com/apache/beam/pull/12117#issuecomment-654253476
@lgajowy thanks a lot for CR. I respond to all comments. Will you find moment to check again?
----------------------------------------------------------------
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
[GitHub] [beam] purbanow commented on a change in pull request #12117: [BEAM-10343] Add dispositions for SnowflakeIO.write
Posted by GitBox <gi...@apache.org>.
purbanow commented on a change in pull request #12117:
URL: https://github.com/apache/beam/pull/12117#discussion_r450011779
##########
File path: sdks/java/io/snowflake/src/main/java/org/apache/beam/sdk/io/snowflake/SnowflakeIO.java
##########
@@ -593,7 +606,7 @@ public void populateDisplayData(DisplayData.Builder builder) {
*
* @param table - String with the name of the table.
*/
- public Write<T> withTable(String table) {
+ public Write<T> toTable(String table) {
Review comment:
Sounds reasonable. I will change to `to()`
----------------------------------------------------------------
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