dybyte commented on code in PR #12525:
URL: https://github.com/apache/seatunnel/pull/12525#discussion_r4124957868
##########
seatunnel-api/src/main/java/org/apache/seatunnel/api/table/catalog/AbstractSchema.java:
##########
@@ -69,18 +86,41 @@ public String[] getFieldNames() {
}
public int indexOf(String columnName) {
- return columnNames.indexOf(columnName);
+ Integer index = getColumnIndexCache().get(columnName);
+ return index == null ? -1 : index;
}
public Column getColumn(String columnName) {
return columns.get(indexOf(columnName));
}
public boolean contains(String columnName) {
- return columnNames.contains(columnName);
+ return indexOf(columnName) != -1;
}
public List<Column> getColumns() {
return Collections.unmodifiableList(columns);
}
+
+ /**
+ * Builds the name to index cache only after the whole map is ready.
Duplicate names keep the
+ * first column so lookups match the former linear scan semantics.
+ */
+ private Map<String, Integer> getColumnIndexCache() {
+ Map<String, Integer> cache = columnIndexCache;
+ if (cache == null) {
+ synchronized (this) {
+ cache = columnIndexCache;
+ if (cache == null) {
+ Map<String, Integer> indexes = new
HashMap<>(columnNames.size());
+ for (int i = 0; i < columnNames.size(); i++) {
+ indexes.putIfAbsent(columnNames.get(i), i);
+ }
+ cache = indexes;
+ columnIndexCache = cache;
+ }
+ }
+ }
+ return cache;
+ }
Review Comment:
Is the lock needed? The map is deterministic and immutable after
publication, so a racy single-check with the volatile write should be enough.
##########
seatunnel-api/src/main/java/org/apache/seatunnel/api/table/catalog/AbstractSchema.java:
##########
@@ -39,9 +44,21 @@ public class AbstractSchema implements Serializable {
@Getter(AccessLevel.PRIVATE)
protected final List<String> columnNames;
+ /** Lazily built column name to index cache, keeps schema lookups constant
time. */
+ @Getter(AccessLevel.NONE)
+ @Setter(AccessLevel.NONE)
+ @ToString.Exclude
+ @EqualsAndHashCode.Exclude
+ private transient volatile Map<String, Integer> columnIndexCache;
+
public AbstractSchema(List<Column> columns) {
- this.columns = columns;
- this.columnNames =
columns.stream().map(Column::getName).collect(Collectors.toList());
+ // Copy the lists so later mutation of the caller's list cannot change
this schema or
+ // invalidate the lazily built lookup caches.
+ this.columns =
+ columns == null
+ ? Collections.emptyList()
+ : Collections.unmodifiableList(new
ArrayList<>(columns));
+ this.columnNames =
this.columns.stream().map(Column::getName).collect(Collectors.toList());
}
Review Comment:
This used to throw an NPE, but now silently becomes an empty schema. Is that
intended? If not, `Objects.requireNonNull` might be safer, since a null here is
likely a caller bug.
--
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]
