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 in CHANGES.md (103, 104, 140, 141)? 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) and it's a bit mess. It would also be great to move snowflake.py from apache_beam.io.external to apache_beam.io to avoid backward compatibility issues, it's just moving this file, there are no other files that depend on it (integration tests and cross-language write are 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 not a lot 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 in 2.24 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). 2) would be of similiar complexity and will avoid backwards compatibility issues. 3) would require to take files from the 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 CHANGES.md: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)).``` Sorry for the troubles and thanks in advance! Whatever option you choose it will save users a lot of 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]
