nastra commented on code in PR #17413: URL: https://github.com/apache/iceberg/pull/17413#discussion_r3782005380
########## core/src/main/java/org/apache/iceberg/expressions/InclusiveStatsEvaluator.java: ########## @@ -0,0 +1,194 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.expressions; Review Comment: @rdblue for completeness, here's Claude's summary of why RevAPI marks those API breakages: The reported breaks are an artifact of these two interfaces having already shipped in **1.11.0** as package-private. The baseline is set in build.gradle via `oldVersion = "1.11.0"`, so revapi resolves the published `org.apache.iceberg:iceberg-core:1.11.0` artifact from Maven and diffs it against the `1.12.0-SNAPSHOT` jar we just built. That's the correct base version for a change targeting `1.12.0` — we're measuring against the last release, not against the merge base. Now, why `old: <none>` is used for comparison: `ContentStats` and `FieldStats` are not new types. They were moved into core by #15971, which landed before `1.11.0`, so both class files are in the `1.11.0` jar — just without the public modifier. Revapi pairs elements up hierarchically. It matches types by fully-qualified name, and since `org.apache.iceberg.ContentStats` exists in both jars, it finds a pair and descends into the members, matching methods by name and parameter types. For a matched pair, any difference is then dropped if the old side wasn't public, because a non-public member was never part of the old API. For a method with no counterpart at all, there is no old side to filter against, so revapi evaluates the new element on its own — a public interface method — and reports `java.method.addedToInterface` with `old: <none>` and `From old archive: <none>`. That's not revapi failing to find the archive; it's revapi telling us this particular method has no predecessor in the old API. That rule accounts for exactly the nine findings we see. They're the nine method names that didn't exist in 1.11.0. Everything else is silent for the same underlying reason. Methods that kept their name and parameters got paired and suppressed, which is why `FieldStats.type()` switching its return type from `Type` to `Types.StructType`, the counts moving from **boxed Long** to **primitive long**, and `fieldStats()` going from `List` to `Iterable` don't show up. The removals (`statsFor, statsStruct, hasExactBounds, avgValueSize, maxValueSize`) don't either, since their old elements were non-public. Worth noting so nobody reads the length of the list as a risk measure: the accepted-breaks list is shorter than the set of actual shape changes here, not longer. The reason these are safe to accept is the same reason they're reported at all. `addedToInterface` is flagged as **source-breaking** because a downstream class implementing the interface would stop compiling — but a package-private interface can't be implemented, or even referenced, outside `org.apache.iceberg`, so there's no such implementor at `1.11.0` to break. For contrast, `InclusiveStatsEvaluator` is a brand-new public class in this PR and produces zero findings, because when the containing type is itself unmatched revapi treats the whole subtree as an addition instead of walking its methods. Had `ContentStats` and `FieldStats` not existed in `1.11.0` at all, promoting them would have been completely silent. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
