Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/22728 )
Change subject: IMPALA-13738 (Part2) WIP: Clean up code in Catalog's table and partition interfaces ...................................................................... Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/22728/2/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: http://gerrit.cloudera.org:8080/#/c/22728/2/fe/src/main/java/org/apache/impala/catalog/Table.java@135 PS2, Line 135: protected SqlConstraints sqlConstraints_ = new SqlConstraints(new ArrayList<>(), I'm thinking about this field. The situation before this cleanup was definitely not good, where we had this 'sqlConstraints_' here as a private field and HdfsTable had another one which shadowed it. As far as I could see, only HdfsTable sets this field, the other subclasses of Table don't reference it. So I thought this field could go directly into HdfsTable and could be deleted from here. But FeTable, which Table implements, mandates the getSqlConstraints() method, which is now implemented here, in Table, by returning 'sqlConstraints_'. AFAICS this will always be an empty SqlConstraints object (the one constructed here) for subclasses other than HdfsTable. A possibility I propose would be that FeTable could default-implement getSqlConstraints(), returning a newly constructed empty SqlConstraints object. The downside is that it would allocate new objects on each call. We could also have an empty SqlConstraints object as a static constant in the FeTable interface (it needs to be static because it is an interface), but then if, in the future, one of the Table subclasses wanted to load/use SqlConstraints, it is possible that instead of creating its own 'sqlConstraints_' field it would get it from FeTable.getSqlConstraints(), and then modify it (because Java doesn't have real constants), but then it would be modified for other objects/classes too. Therefore I don't think it's a good idea. Noémi, Steven, Zoltán, what do you think of always returning a new, empty SqlConstraints object in a default FeTable.getSqlConstraints()? Do you think we can afford that? -- To view, visit http://gerrit.cloudera.org:8080/22728 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8d9c7ba876e39fa4f4d067761f25dc4ecfd55702 Gerrit-Change-Number: 22728 Gerrit-PatchSet: 2 Gerrit-Owner: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Steve Carlin <scar...@cloudera.com> Gerrit-Comment-Date: Fri, 04 Apr 2025 12:35:23 +0000 Gerrit-HasComments: Yes
