Add checkstyle rule for uppercase constant fields #10673

attilakreiner · 2024-07-10T09:00:33Z

No description provided.

nastra · 2024-07-10T10:36:27Z

@attilakreiner thanks for doing this and generally this looks good.

Could you please extract the Spark + Flink changes into a separate PR (where checkstyle doesn't enforce this naming yet)? I'll review the Spark + Flink changes first (as those typically get quite large) and once those changes are in, we can continue with this PR (containing the changes for all other modules + enforcing the naming).

attilakreiner · 2024-07-10T12:19:18Z

@nastra, got it. Done.

I reverted the flink+spark changes in this PR.
I created this PR Flink, Spark: Rename constants to be all uppercase #10675 for the flink+spark changes.

findepi · 2024-07-10T12:52:04Z

.baseline/checkstyle/checkstyle.xml

@@ -284,6 +284,10 @@
            <property name="format" value="^[a-z][a-zA-Z0-9]+$"/>
            <message key="name.invalidPattern" value="Member name ''{0}'' must match pattern ''{1}''."/>
        </module>
+        <module name="ConstantName">
+            <property name="format" value="^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$"/>


you can use possessive quantifiers. That way the matching might be simpler.
I didn't benchmark it, but checksyle can take considerable time overall, and small things like this add up.

I updated the regexp from
^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$
to
^[A-Z][A-Z0-9]+(_[A-Z0-9]+)*$
Pls mind the * vs + char at pos 15. Pls clarify whether or not this is what you were pointing out.

Update: I reverted this change as it breaks for Z_COLUMN, Z_SCHEMA and Z_SORT_ORDER.

Pls point out if you suggest a different regexp here. 🤔

to leverage possessive quantifiers replace

^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$

with

^[A-Z][A-Z0-9]*+(_[A-Z0-9]++)*+$

Thanks, I see. I experimented a bit with the possessive quantifiers in the regexes as well as removing some custom messages that seem redundant.

I created a draft PR with these changes: #10681. Even though it seems like just a few lines of changes, it has an impact for the whole codebase and I don't want to hold up this one. (I'd prefer to keep the scope of this PR tight, really just renaming the constants to uppercase and enforcing that rule.)

I think for this PR we should just stick with the default regex pattern of checkstyle, which I believe is what you had initially. Improving the regex can be done in a separate PR.

@nastra, yep, same thought here, hence the spin-off draft PR.

In this PR the definition I used is this (this is the current state right now):
<property name="format" value="^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$"/>

the default you linked is this:
<property name="format" value="^log(ger)?$|^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$"/>

So the only difference is I removed the first part that allows for the lowercase log(ger) as this codebase doesn't use it, so it was redundant.

Also we have the custom message in place to be consistent with the rest of the definitions for now.

Pls LMK if this is all good as it is right now or we need some changes here.

findepi · 2024-07-10T12:53:04Z

.baseline/checkstyle/checkstyle.xml

@@ -284,6 +284,10 @@
            <property name="format" value="^[a-z][a-zA-Z0-9]+$"/>
            <message key="name.invalidPattern" value="Member name ''{0}'' must match pattern ''{1}''."/>
        </module>
+        <module name="ConstantName">
+            <property name="format" value="^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$"/>
+            <message key="name.invalidPattern" value="Constant name ''{0}'' must match pattern ''{1}''."/>


Is this property needed? What error message does checkstyle produce when we omit it?

The message without this property is:
Name 'factory' must match pattern '^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$'. [ConstantName]
with this property it's:
Constant name 'factory' must match pattern '^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$'. [ConstantName]

My preference is to keep it as both the definition and the output is more consistent with the existing rules.

Both messages are clear and actionable. My preference would be not to add configuration that is not needed.
I noticed we do set this for other checks, and I believe we can remove those other configurations too, in places where they do not add much value.

Got it. Pls see my previous comment about #10681.

This reverts commit 85ce782.

attilakreiner · 2024-07-10T15:02:17Z

Merged in the main branch.

snazy · 2024-07-10T15:41:07Z

mr/src/main/java/org/apache/hadoop/hive/ql/exec/vector/VectorizedSupport.java

@@ -34,11 +34,11 @@ public enum Support {
      this.lowerCaseName = name().toLowerCase(Locale.ROOT);
    }

-    public static final Map<String, Support> nameToSupportMap = Maps.newHashMap();
+    public static final Map<String, Support> NAME_TO_SUPPORT_MAP = Maps.newHashMap();


This changes a publicly accessible field - a breaking ABI change.

Thanks for pointing it out. I reverted this change. I think it's fair to think about this one as an exceptional case, so I added @SuppressWarnings("checkstyle:ConstantName") to overcome the issue. Pls LMK if this fix is OK.

I double checked the PR for similar cases and I did not find any more public static fields that were affected.

just adding some additional context that Iceberg only provides API/ABI stability for certain modules (iceberg-mr module isn't included) which is enforced by revapi, but great if we can avoid breaking the API/ABI for modules where we don't enforce it

nastra

LGTM, thanks @attilakreiner

Add checkstyle rule for uppercase constant fields

15e07a5

github-actions bot added API spark parquet arrow core data flink MR pig INFRA hive AWS NESSIE ALIYUN labels Jul 10, 2024

Revert flink and spark changes

0fa3c7e

findepi reviewed Jul 10, 2024

View reviewed changes

attilakreiner added 3 commits July 10, 2024 16:52

Update regexp for ConstantName

85ce782

Merge branch 'main' into constant-uppercase

998032f

Revert "Update regexp for ConstantName"

115b29d

This reverts commit 85ce782.

snazy reviewed Jul 10, 2024

View reviewed changes

Revert change in VectorizedSupport

e59fcb7

attilakreiner mentioned this pull request Jul 11, 2024

Update checkstyle definition #10681

Merged

nastra approved these changes Jul 11, 2024

View reviewed changes

nastra merged commit 41e1951 into apache:main Jul 11, 2024
50 checks passed

jasonf20 pushed a commit to jasonf20/iceberg that referenced this pull request Aug 4, 2024

Rename & enforce constants to be all uppercase (apache#10673)

5bab775

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add checkstyle rule for uppercase constant fields #10673

Add checkstyle rule for uppercase constant fields #10673

attilakreiner commented Jul 10, 2024

nastra commented Jul 10, 2024

attilakreiner commented Jul 10, 2024 •

edited

Loading

findepi Jul 10, 2024

attilakreiner Jul 10, 2024

attilakreiner Jul 10, 2024

findepi Jul 11, 2024

attilakreiner Jul 11, 2024

nastra Jul 11, 2024

attilakreiner Jul 11, 2024 •

edited

Loading

findepi Jul 10, 2024

attilakreiner Jul 10, 2024

findepi Jul 11, 2024

attilakreiner Jul 11, 2024 •

edited

Loading

attilakreiner commented Jul 10, 2024

snazy Jul 10, 2024

attilakreiner Jul 10, 2024 •

edited

Loading

attilakreiner Jul 10, 2024

nastra Jul 10, 2024

snazy Jul 10, 2024

nastra left a comment

Add checkstyle rule for uppercase constant fields #10673

Add checkstyle rule for uppercase constant fields #10673

Conversation

attilakreiner commented Jul 10, 2024

nastra commented Jul 10, 2024

attilakreiner commented Jul 10, 2024 • edited Loading

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

attilakreiner Jul 11, 2024 • edited Loading

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

attilakreiner Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

attilakreiner commented Jul 10, 2024

Choose a reason for hiding this comment

attilakreiner Jul 10, 2024 • edited Loading

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

nastra left a comment

Choose a reason for hiding this comment

attilakreiner commented Jul 10, 2024 •

edited

Loading

attilakreiner Jul 11, 2024 •

edited

Loading

attilakreiner Jul 11, 2024 •

edited

Loading

attilakreiner Jul 10, 2024 •

edited

Loading