piotr-szuberski commented on a change in pull request #12758: URL: https://github.com/apache/beam/pull/12758#discussion_r482784051
########## File path: CHANGES.md ########## @@ -100,6 +100,8 @@ * Support for X source added (Java/Python) ([BEAM-X](https://issues.apache.org/jira/browse/BEAM-X)). * Add streaming support to SnowflakeIO in Java SDK ([BEAM-9896](https://issues.apache.org/jira/browse/BEAM-9896 )) * Support reading and writing to Google Healthcare DICOM APIs in Python SDK ([BEAM-10601](https://issues.apache.org/jira/browse/BEAM-10601)) +* Add dispositions for SnowflakeIO.write ([BEAM-10343](https://issues.apache.org/jira/browse/BEAM-10343)) Review comment: @youngoli Could I also ask you to change these lines (103, 104, 140, 141 as well? I've noticed that snowflake dispositions and cross-language read was added in 2.24, not 2.23 (it was pushed at 2.23 but was merged after the 2.23 release) It would also be great to move snowflake.py from apache_beam.io.external to apache_beam.io to avoid backward compatibility issues, but I don't know how much changes would you like to do. It's just moving this file, there are no other files that depend on it (integration tests and cross-language write will be added in 2.25.0). On the other hand we could add apache_beam/io/snowflake.py and apache_beam/io/external/xlang_snowflakeio_it_test.py from the current master to the 2.24.0 RC and remove apache_beam/io/external/snowflake.py - it would be no more work, these tests don't run on jenkins, so they won't break anything. We would have snowflake read and write for python at once without much work. But I probably am asking for too much at once - I don't know how much are you willing to do so I understand if you decide to stop at 1) - but 2) would be of similiar complexity. 3) would require to take changes from current master but is also easy. TL;DR - choose one of them: 1. just remove lines 140-141 and add 103-104 2. do 1) and move apache_beam/io/external/snowflake.py to apache_beam/io/snowflake.py - then in CHANGES.md:104 should be `apache_beam.io.snowflake` instead of `apache_beam.io.external.snowflake` 3. Take following from current master `apache_beam/io/snowflake.py` and `apache_beam/io/external/xlang_snowflakeio_it_test.py`, remove `apache_beam/io/external/snowflake.py` and do 1), but in line 104 paste ```* Add cross-language support to SnowflakeIO now available in the Python module `apache_beam.io.snowflake` ([BEAM-9897](https://issues.apache.org/jira/browse/BEAM-9897), [BEAM-9898](https://issues.apache.org/jira/browse/BEAM-9898)).``` Thanks in advance! It will save users some confusion! ---------------------------------------------------------------- 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: [email protected]
