Jackie-Jiang commented on code in PR #19581:
URL: https://github.com/apache/pinot/pull/19581#discussion_r4319540648
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java:
##########
@@ -1219,6 +1321,345 @@ private void putDictionaryCompressionStats(String
column, long uncompressedValue
compressionMetadata.applyTo(_segmentProperties, column);
}
+ /// Reorders the actions so that a column is created after every column it
reads that is also being created in
+ /// this run -- the dependency that makes chained derived columns work, and
that also lets a derived column read a
+ /// plain default column added in the same reload. Non-derived columns keep
their original relative order.
+ ///
+ /// A dependency cycle cannot be satisfied in any order, so the columns in
it are emitted last, unordered: each
+ /// still gets created, falling back to its default value because its
argument is not in the segment yet. The
+ /// cycle is logged rather than failing the segment load.
+ private Map<String, DefaultColumnAction> orderByDependencies(Map<String,
DefaultColumnAction> actionMap) {
+ Map<String, List<String>> dependencies = new HashMap<>();
+ for (Map.Entry<String, DefaultColumnAction> entry : actionMap.entrySet()) {
+ if (!entry.getValue().isRemoveAction()) {
+ List<String> arguments = derivedColumnArguments(entry.getKey());
+ if (arguments != null) {
+ List<String> pending = new ArrayList<>(arguments.size());
+ for (String argument : arguments) {
+ // Only columns written in this run constrain the order; anything
already in its final shape is readable.
+ // An updated argument counts as much as an added one: its values
are only correct once its own action has
+ // run, so reading it earlier would bake the pre-update values
into the derived column for good.
+ DefaultColumnAction argumentAction = actionMap.get(argument);
+ if (argumentAction != null && !argumentAction.isRemoveAction() &&
!argument.equals(entry.getKey())) {
+ pending.add(argument);
+ }
+ }
+ if (!pending.isEmpty()) {
+ dependencies.put(entry.getKey(), pending);
+ }
+ }
+ }
+ }
+ if (dependencies.isEmpty()) {
+ return actionMap;
+ }
+
+ markCyclicColumns(dependencies);
+
+ Map<String, DefaultColumnAction> ordered = new
LinkedHashMap<>(actionMap.size());
+ for (String column : actionMap.keySet()) {
+ visitForOrdering(column, actionMap, dependencies, ordered);
+ }
+ // Columns dropped by a cycle still have to be created; they land last and
take the default-value path.
+ for (Map.Entry<String, DefaultColumnAction> entry : actionMap.entrySet()) {
+ ordered.putIfAbsent(entry.getKey(), entry.getValue());
+ }
+ return ordered;
+ }
+
+ private void visitForOrdering(String column, Map<String,
DefaultColumnAction> actionMap,
+ Map<String, List<String>> dependencies, Map<String, DefaultColumnAction>
ordered) {
+ // A column on a cycle is left out here and appended at the end, so that
anything reading it is created first and
+ // takes the fallback on a missing argument instead of deriving a value
from a sibling's default. Skipping those
+ // columns is also what bounds this traversal: every edge that closes a
cycle leads into one of them.
+ if (ordered.containsKey(column) || _cyclicColumns.contains(column) ||
!actionMap.containsKey(column)) {
Review Comment:
**[P2] Explicitly handle dependents of cyclic columns being updated**
Postponing cyclic columns assumes that their dependents will encounter
missing source metadata and take the fallback. That is not true when the cyclic
columns already exist and are being updated. For example, existing
autogenerated `a=1,b=2` change their schema defaults to `10,20`, with
transforms `a=plus(b,1)` and `b=plus(a,1)`, while a new `c=plus(a,1)` is added.
This ordering builds `c` first; `getColumnMetadata(a)` finds the old column, so
`c` permanently stores `2`. Only afterward do `a,b` receive `10,20`. Explicitly
handle cyclic arguments when resolving a dependent column instead of relying on
absent metadata to trigger the intended fallback, and cover updated cyclic
sources with a new dependent. This is non-blocking.
--
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]
