[FLINK-30481][FLIP-277] GlueCatalog Implementation #47

Samrat002 · 2023-01-12T10:54:29Z

Purpose of the change

Add Glue catalog feature

Verifying this change

Tested in EMR cluster

Unit test cases

Significant changes

(Please check any boxes [x] if the answer is "yes" )

Dependencies have been added or upgraded
Public API has been changed (Public API is any class annotated with @Public(Evolving))
Serializers have been changed
New feature has been introduced
- If yes, how is this documented? (not applicable / docs / JavaDocs / not documented)

...catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/mapper/GlueDatatypeMapper.java

...k-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/GlueCatalogFactory.java

flink-catalog-aws-glue/pom.xml

flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/util/GlueOperator.java

Samrat002 · 2023-02-17T02:44:42Z

@dannycranmer please review changes in free time

luoyuxia

@Samrat002 Thanks for bring this feature. I'm not familiar with GlueCatalog, but I left some comments from Flink and Flink Catalog sides.

flink-catalog-aws-glue/pom.xml

flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/GlueCatalog.java

...k-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/util/AwsProperties.java

...catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/mapper/GlueDatatypeMapper.java

...atalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/util/AwsClientFactory.java

...catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/mapper/GlueDatatypeMapper.java

flink-catalog-aws-glue/src/test/java/org/apache/flink/table/catalog/GlueCatalogTest.java

flink-catalog-aws-glue/pom.xml

flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/GlueCatalog.java

flink-catalog-aws-glue/pom.xml

...catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/GlueCatalogConstants.java

...atalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/util/AwsClientFactory.java

...alog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/util/AwsClientFactories.java

flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/util/GlueOperator.java

Samrat002 · 2023-09-09T06:06:48Z

@dannycranmer @vahmed-hamdy please review wheenver time

Samrat002 · 2023-09-27T05:39:22Z

@dannycranmer @vahmed-hamdy please review whenever time 🙏🏻

Samrat002 · 2023-10-26T12:28:31Z

[gentle ping]
@dannycranmer, i have addressed to all the review comments . Please review the PR whenever time

Samrat002 · 2023-11-02T17:32:21Z

@PrabhuJoseph please review the PR whenever time

wckdman · 2024-03-12T11:14:55Z

Any news here? :)
I'm really looking forward to see this feature 🙏

vikramsinghchandel · 2024-05-13T14:43:20Z

any updates here? This is a feature that is already available on Flink EMR would be great to have it here too.

Samrat002 · 2024-07-04T04:37:16Z

vikramsinghchandel@ wckdman@ . thank you for your interest
i will find a reviewer and close the feature

Samrat002 · 2024-07-06T06:44:01Z

@dannycranmer @fapaul Please review whenever time

foxus

Many thanks for your efforts with this one @Samrat002 - I've made a few suggestions.

...ws/flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/GlueCatalog.java

...flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/util/GlueUtils.java

Samrat002 · 2024-07-25T18:45:42Z

Thank you @foxus for the review .
this PR was opened long ago. Let me try to address all the comments and test it again e2e .
this PR will be ready for review by next 2 days

foxus · 2024-07-29T21:24:47Z

Thank you @foxus for the review .

this PR was opened long ago. Let me try to address all the comments and test it again e2e .

this PR will be ready for review by next 2 days

Many thanks, let me know when it's ready and I'll have another look. I noticed there were a couple of previous unaddressed comments from other reviewers - it will be great to get this one merged in.

foxus · 2024-08-07T08:18:50Z

Thank you @foxus for the review .

this PR was opened long ago. Let me try to address all the comments and test it again e2e .

this PR will be ready for review by next 2 days

Hey, let me know if I can help progress this? I'm happy to assist if you're willing and able to give me write access to the branch.

Samrat002 · 2024-08-10T06:20:20Z

@foxus
Apologies for the delay. i have mostly addressed to all the comments . Lastly wrapping up validating the working .
i will ping you once the pr is ready for review

Samrat002 · 2024-08-10T06:35:15Z

let me know if I can help progress this? I'm happy to assist if you're willing and able to give me write access to the branch.

yes . send invite for colaboration

Samrat002 · 2024-08-12T04:11:11Z

@foxus Please review whenever time

foxus · 2024-08-14T22:44:59Z

@foxus Please review whenever time

Thanks for the effort responding to the comments, there's only one which I think has been marked as resolved but still appears not to have been addressed.

Samrat002 · 2024-08-15T08:58:50Z

Fixed the missing comment.
it might have slipped unintentionally

@foxus please review

foxus

Apologies @Samrat002, I've spotted two more blocking bugs that I should have spotted before.

foxus · 2024-08-17T23:50:18Z

...aws/flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/TypeMapper.java

+                return DataTypes.TIMESTAMP(5);
+            case "array":
+                // Example: array<string> -> DataTypes.ARRAY(DataTypes.STRING())
+                String elementType = glueType.substring(6, glueType.length() - 1);


Forgive me for not spotting this earlier - this will always cause an out of bounds exception. The only way this block of code runs is if glueType is equal to the string literal array but in this block you're attempting to parse the part of the string which cannot exist (array<string>). It looks like you're expecting case here to behave as a startsWith. Please address this and consider adding unit tests for this method.

foxus · 2024-08-17T23:51:40Z

...aws/flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/TypeMapper.java

+                // Example: map<string, string> -> DataTypes.MAP(DataTypes.STRING(),
+                // DataTypes.STRING())
+                int commaIndex = glueType.indexOf(",");
+                String keyType = glueType.substring(4, commaIndex);


Forgive me for not spotting this earlier - this will always cause an out of bounds exception. The only way this block of code runs is if glueType is equal to the string literal map but in this block you're attempting to parse the part of the string which cannot exist (map<string, string>). It looks like you're expecting case here to behave as a startsWith. Please address this and consider adding unit tests for this method. I would consider using regex here to parse the key and value types - it will allow you to be more permissive about whitespace (e.g. map<string, string> and map<string,string>) and also allow you to play with named groups (a personal favourite).

foxus · 2024-08-18T00:35:56Z

@Samrat002, thanks for agreeing to collaborate, I've resolved some of the comments but left it as a separate commit to allow you to review and squash if you're happy whilst you're addressing the TypeMapper issue.

Co-Authored-By: Anthony Pounds-Cornish <[email protected]>

Samrat002 · 2024-08-18T17:45:56Z

@foxus thank you for adding the change. 👍🏻
please review again i have added tests and tried to address to your suggested change.

vahmed-hamdy

I have partially reviewed the PR, I will try to resume during this week, the PR has been hanging for a very long time and is of huge size, Do you believe it might be easier to break it down to a couple of PRs, so it is easier to review and more importantly to provide thorough testing for each part.
I suggest starting with laying the code structure and minimum possible set of features and exposing minimum configuration while defaulting the rest.
We can add features like functions and partitions and http-client configurations in a following PR.

vahmed-hamdy · 2024-08-17T19:57:40Z

flink-catalog-aws/flink-catalog-aws-glue/pom.xml

+    </parent>
+
+    <artifactId>flink-catalog-aws-glue</artifactId>
+    <name>Flink : Catalog : AWS : Glue</name>


Suggested change

<name>Flink : Catalog : AWS : Glue</name>

<name>Flink : Catalog : AWS : Glue Data Catalog</name>

vahmed-hamdy · 2024-08-17T20:02:56Z

...glue/src/main/java/org/apache/flink/table/catalog/glue/constants/AWSGlueConfigConstants.java

+     * <p>For more details, see <a
+     * href="https://docs.aws.amazon.com/glue/latest/dg/aws-glue-api-catalog-databases.html">...</a>
+     */
+    public static final String GLUE_CATALOG_ID = "aws.glue.id";


could we use aws.glue.catalog-id since we also have account-id

vahmed-hamdy · 2024-08-17T20:07:05Z

...k-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/GlueCatalogOptions.java

+    }
+
+    public static Set<ConfigOption<?>> getRequiredConfigOptions() {
+        return new HashSet<>();


Why not have GLUE_CATALOG_ID and REGION as required options?

vahmed-hamdy · 2024-08-17T20:08:09Z

...k-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/GlueCatalogOptions.java

+    public static final ConfigOption<String> REGION =
+            ConfigOptions.key(AWSConfigConstants.AWS_REGION)
+                    .stringType()
+                    .defaultValue(Region.US_WEST_1.toString());


Why us-west-1 ? I prefer to have no default and make it required like the case in the connectors

vahmed-hamdy · 2024-08-17T20:10:24Z

...k-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/GlueCatalogOptions.java

+import static org.apache.flink.table.catalog.glue.GlueCatalog.DEFAULT_DB;
+
+/** Collection of {@link ConfigOption} used in GlueCatalog. */
+@Internal


this is not Internal suggest to go with PublicEvolving

vahmed-hamdy · 2024-08-18T17:31:53Z

...glue/src/test/java/org/apache/flink/table/catalog/glue/util/GlueCatalogOptionsUtilsTest.java

+class GlueCatalogOptionsUtilsTest {
+
+    @Test
+    void testGetValidatedConfigurations() {}


please remove If not needed

vahmed-hamdy · 2024-08-18T17:32:33Z

...nnector-aws-base/src/main/java/org/apache/flink/connector/aws/config/AWSConfigConstants.java

+     * href="https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/apache/ApacheHttpClient.Builder.html">...</a>
+     */
+    public static final String HTTP_CLIENT_CONNECTION_TIMEOUT_MS =
+            "http-client.connection-timeout-ms";


vahmed-hamdy · 2024-08-18T17:32:59Z

...nnector-aws-base/src/main/java/org/apache/flink/connector/aws/config/AWSConfigConstants.java

+     * <p>For more details, see <a
+     * href="https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/http/apache/ApacheHttpClient.Builder.html">...</a>
+     */
+    public static final String HTTP_CLIENT_SOCKET_TIMEOUT_MS = "http-client.socket-timeout-ms";


same as above

vahmed-hamdy · 2024-08-18T17:33:13Z

...-aws-base/src/main/java/org/apache/flink/connector/aws/table/util/HttpClientOptionUtils.java

+    private static final String CLIENT_HTTP_PROTOCOL_VERSION_OPTION = "protocol.version";
+    private static final String CLIENT_HTTP_MAX_CONNECTION_TIMEOUT_MS = "connection-timeout-ms";
+    private static final String CLIENT_HTTP_MAX_SOCKET_TIMEOUT_MS = "socket-timeout-ms";
+    private static final String APACHE_MAX_CONNECTIONS = "apache.max-connections";


vahmed-hamdy · 2024-08-18T17:33:56Z

flink-catalog-aws/flink-catalog-aws-glue/src/main/resources/META-INF/NOTICE

@@ -0,0 +1,7 @@
+flink-catalog-aws-glue
+Copyright 2014-2023 The Apache Software Foundation


vahmed-hamdy · 2024-08-19T11:15:09Z

...aws/flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/TypeMapper.java

+ * @see org.apache.flink.table.catalog.CatalogTable
+ * @see org.apache.flink.table.catalog.ResolvedCatalogTable
+ */
+public class TypeMapper {


annotation missing

vahmed-hamdy · 2024-08-19T11:19:49Z

...flink-catalog-aws-glue/src/test/java/org/apache/flink/table/catalog/glue/TypeMapperTest.java

+public class TypeMapperTest {
+
+    @Test
+    public void testMapFlinkTypeToGlueType_Primitives() {


nit: Junit5 conventions don't suggest a prefix test in test names
https://junit.org/junit5/docs/current/user-guide/

vahmed-hamdy · 2024-08-19T11:20:31Z

...flink-catalog-aws-glue/src/test/java/org/apache/flink/table/catalog/glue/TypeMapperTest.java

+    }
+
+    @Test
+    public void testMapFlinkTypeToGlueType_Array() {


nit: We can use parameterized tests for all of these tests

vahmed-hamdy · 2024-08-19T11:20:51Z

...flink-catalog-aws-glue/src/test/java/org/apache/flink/table/catalog/glue/TypeMapperTest.java

+    @Test
+    public void testGlueTypeToFlinkType_Array() {
+        LogicalType arrayType = new ArrayType(new VarCharType(255));
+        assertEquals("array<string>", TypeMapper.mapFlinkTypeToGlueType(arrayType));


this is not glue to Flink as test suggests

vahmed-hamdy · 2024-08-19T11:20:59Z

...flink-catalog-aws-glue/src/test/java/org/apache/flink/table/catalog/glue/TypeMapperTest.java

+    @Test
+    public void testGlueTypeToFlinkType_Map() {
+        LogicalType mapType = new MapType(new VarCharType(255), new IntType());
+        assertEquals("map<string,int>", TypeMapper.mapFlinkTypeToGlueType(mapType));


this is not glue to Flink as test suggests

vahmed-hamdy · 2024-08-19T11:45:30Z

flink-catalog-aws/flink-catalog-aws-glue/pom.xml

+        </dependency>
+
+    </dependencies>
+


IIUC, this should be distributed as an uberjar, right?
Do we need to relocate the sdk dependencies?

vahmed-hamdy · 2024-08-19T11:46:06Z

flink-catalog-aws/flink-catalog-aws-glue/src/main/resources/META-INF/NOTICE

+This product includes software developed at
+The Apache Software Foundation (http://www.apache.org/).
+
+This project bundles the following dependencies under the Apache Software License 2.0. (http://www.apache.org/licenses/LICENSE-2.0.txt)


Is this file needed? Unless we bundle and relocate any dependencies I don't think we need it.

vahmed-hamdy · 2024-08-19T11:49:51Z

...flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/util/GlueUtils.java

+     * @param properties Map of properties.
+     * @return fully qualified owner name.
+     */
+    public static String extractTableOwner(Map<String, String> properties) {


nit: Ideally we return Optional and add it to builder if present

vahmed-hamdy · 2024-08-19T11:50:13Z

...flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/util/GlueUtils.java

+     * @param properties Map of properties.
+     * @return fully qualified owner name.
+     */
+    public static String extractTableOwner(Map<String, String> properties) {


Why do we remove?

removing is not tested

vahmed-hamdy · 2024-08-19T11:54:45Z

...g-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/operator/GlueTableOperator.java

+                        .tableType(table.getTableKind().name())
+                        .lastAccessTime(Instant.now())
+                        .owner(tableOwner)
+                        .viewExpandedText(GlueUtils.getExpandedQuery(table))


since this is not a mandatory parameter in builder and we don't really support those yet, why do we set those?

Samrat002 marked this pull request as draft January 12, 2023 10:54

Samrat002 force-pushed the FLINK-30481 branch 8 times, most recently from 665271b to 4ad9f96 Compare January 21, 2023 06:50

Samrat002 force-pushed the FLINK-30481 branch 2 times, most recently from 43308aa to ccf35dc Compare January 24, 2023 12:57

Samrat002 force-pushed the FLINK-30481 branch from ccf35dc to ef5e967 Compare January 31, 2023 10:58

Samrat002 marked this pull request as ready for review January 31, 2023 10:58

Samrat002 changed the title ~~[FLINK-30481][DRAFT][FLIP-277] GlueCatalog Implementation~~ [FLINK-30481][FLIP-277] GlueCatalog Implementation Jan 31, 2023

guptashailesh92 reviewed Jan 31, 2023

View reviewed changes

...catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/mapper/GlueDatatypeMapper.java Outdated Show resolved Hide resolved

guptashailesh92 reviewed Jan 31, 2023

View reviewed changes

...k-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/GlueCatalogFactory.java Outdated Show resolved Hide resolved

guptashailesh92 reviewed Jan 31, 2023

View reviewed changes

flink-catalog-aws-glue/pom.xml Outdated Show resolved Hide resolved

guptashailesh92 reviewed Jan 31, 2023

View reviewed changes

flink-catalog-aws-glue/src/main/java/org/apache/flink/table/catalog/glue/util/GlueOperator.java Outdated Show resolved Hide resolved

Samrat002 force-pushed the FLINK-30481 branch 3 times, most recently from 2a82242 to 9117402 Compare February 13, 2023 05:54

Samrat002 requested a review from guptashailesh92 February 17, 2023 02:45

luoyuxia reviewed Feb 28, 2023

View reviewed changes

Samrat002 force-pushed the FLINK-30481 branch 2 times, most recently from 0e2dc19 to 10dfc99 Compare March 16, 2023 12:21

Samrat002 requested review from luoyuxia and removed request for guptashailesh92 March 16, 2023 13:22

Samrat002 commented Mar 16, 2023

View reviewed changes

flink-catalog-aws-glue/src/test/java/org/apache/flink/table/catalog/GlueCatalogTest.java Outdated Show resolved Hide resolved

dannycranmer requested changes Mar 24, 2023

View reviewed changes

Samrat002 force-pushed the FLINK-30481 branch from 6bf2885 to 6f678d1 Compare July 6, 2024 06:42

foxus suggested changes Jul 24, 2024

View reviewed changes

Samrat002 force-pushed the FLINK-30481 branch from 6f678d1 to 12e9d66 Compare August 10, 2024 14:02

Samrat002 requested a review from foxus August 10, 2024 19:56

Samrat002 force-pushed the FLINK-30481 branch from 457bf76 to 71f6374 Compare August 15, 2024 17:15

foxus suggested changes Aug 17, 2024

View reviewed changes

[FLINK-30481][FLIP-277] GlueCatalog Implementation

7cb8122

Co-Authored-By: Anthony Pounds-Cornish <[email protected]>

Samrat002 force-pushed the FLINK-30481 branch from 8cf1627 to 7cb8122 Compare August 18, 2024 17:44

Samrat002 requested review from foxus and guptashailesh92 August 18, 2024 17:46

vahmed-hamdy suggested changes Aug 18, 2024

View reviewed changes

vahmed-hamdy suggested changes Aug 19, 2024

View reviewed changes

	<name>Flink : Catalog : AWS : Glue</name>
	<name>Flink : Catalog : AWS : Glue Data Catalog</name>

		@@ -0,0 +1,7 @@
		flink-catalog-aws-glue
		Copyright 2014-2023 The Apache Software Foundation

[FLINK-30481][FLIP-277] GlueCatalog Implementation #47

Are you sure you want to change the base?

[FLINK-30481][FLIP-277] GlueCatalog Implementation #47

Conversation

Samrat002 commented Jan 12, 2023 • edited Loading

Purpose of the change

Verifying this change

Significant changes

Samrat002 commented Feb 17, 2023

luoyuxia left a comment

Choose a reason for hiding this comment

Samrat002 commented Sep 9, 2023

Samrat002 commented Sep 27, 2023

Samrat002 commented Oct 26, 2023

Samrat002 commented Nov 2, 2023

wckdman commented Mar 12, 2024

vikramsinghchandel commented May 13, 2024

Samrat002 commented Jul 4, 2024

Samrat002 commented Jul 6, 2024

foxus left a comment • edited Loading

Choose a reason for hiding this comment

Samrat002 commented Jul 25, 2024

foxus commented Jul 29, 2024 • edited Loading

foxus commented Aug 7, 2024

Samrat002 commented Aug 10, 2024

Samrat002 commented Aug 10, 2024

Samrat002 commented Aug 12, 2024

foxus commented Aug 14, 2024

Samrat002 commented Aug 15, 2024 • edited Loading

foxus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foxus commented Aug 18, 2024

Samrat002 commented Aug 18, 2024

vahmed-hamdy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Samrat002 commented Jan 12, 2023 •

edited

Loading

foxus left a comment •

edited

Loading

foxus commented Jul 29, 2024 •

edited

Loading

Samrat002 commented Aug 15, 2024 •

edited

Loading