julianhyde commented on code in PR #3984:
URL: https://github.com/apache/calcite/pull/3984#discussion_r1801769170
##########
core/src/main/java/org/apache/calcite/sql/validate/SemanticTable.java:
##########
@@ -44,4 +50,21 @@ public interface SemanticTable {
default boolean mustFilter(int column) {
return getFilter(column) != null;
}
+
+ /**
+ * Returns a list of column ordinals (0-based) of fields that defuse
+ * must-filter columns when filtered on.
+ */
+ default List<Integer> bypassFieldList() {
+ return emptyList();
Review Comment:
Use `ImmutableList.of()` not `Collections.emptyList()`.
##########
core/src/main/java/org/apache/calcite/sql/validate/AbstractNamespace.java:
##########
@@ -62,6 +64,10 @@ abstract class AbstractNamespace implements
SqlValidatorNamespace {
* should typically be re-assigned on validate. */
protected ImmutableBitSet mustFilterFields = ImmutableBitSet.of();
+ protected ImmutableBitSet mustFilterBypassFields = ImmutableBitSet.of();
Review Comment:
fields need javadoc, like `mustFilterFields`
##########
testkit/src/main/java/org/apache/calcite/test/catalog/MockCatalogReader.java:
##########
@@ -1026,27 +1028,32 @@ public static class MockDynamicTable
public static class MustFilterMockTable
extends MockTable implements SemanticTable {
private final Map<String, String> fieldFilters;
+ private final List<Integer> bypassFieldList;
+
Review Comment:
remove blank line
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -1217,12 +1219,21 @@ protected void validateNamespace(final
SqlValidatorNamespace namespace,
// fields; these are neutralized if the consuming query filters on
them.
final ImmutableBitSet mustFilterFields =
namespace.getMustFilterFields();
- if (!mustFilterFields.isEmpty()) {
+ // Remnant must filter fields are fields that are not selected and
cannot
Review Comment:
Not sure whether the 'remnant' concept is necessary. Instead could we just
remove must-filter fields that have been satisfied?
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -1217,12 +1219,21 @@ protected void validateNamespace(final
SqlValidatorNamespace namespace,
// fields; these are neutralized if the consuming query filters on
them.
final ImmutableBitSet mustFilterFields =
namespace.getMustFilterFields();
- if (!mustFilterFields.isEmpty()) {
+ // Remnant must filter fields are fields that are not selected and
cannot
+ // be defused unlessa bypass field defuses it.
+ // A top-level namespace must not have any remnant-must-filter fields.
+ final Set<SqlQualified> remnantMustFilterFields =
namespace.getRemnantMustFilterFields();
+ if (!mustFilterFields.isEmpty() || !remnantMustFilterFields.isEmpty())
{
+ Stream<String> mustFilterStream =
+ StreamSupport.stream(mustFilterFields.spliterator(), false)
+ .map(namespace.getRowType().getFieldNames()::get);
+ Stream<String> remnantStream =
+ StreamSupport.stream(remnantMustFilterFields.spliterator(),
false)
+ .map(q -> q.suffix().get(0));
+
// Set of field names, sorted alphabetically for determinism.
- Set<String> fieldNameSet =
- StreamSupport.stream(mustFilterFields.spliterator(), false)
- .map(namespace.getRowType().getFieldNames()::get)
- .collect(Collectors.toCollection(TreeSet::new));
+ Set<String> fieldNameSet = Stream.concat(mustFilterStream,
remnantStream)
Review Comment:
add linefeed after `=`
##########
core/src/main/java/org/apache/calcite/sql/validate/AbstractNamespace.java:
##########
@@ -169,6 +175,15 @@ abstract class AbstractNamespace implements
SqlValidatorNamespace {
"mustFilterFields (maybe validation is not complete?)");
}
+ @Override public ImmutableBitSet getMustFilterBypassFields() {
+ return requireNonNull(mustFilterBypassFields,
+ "mustFilterBypassFields (maybe validation is not complete?)");
+ }
+
+ @Override public Set<SqlQualified> getRemnantMustFilterFields() {
+ return requireNonNull(remnantMustFilterFields,
+ "remnantMustFilterFields (maybe validation is not complete?");
+ }
Review Comment:
add blank line
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -4185,6 +4217,48 @@ private static void forEachQualified(SqlNode node,
SqlValidatorScope scope,
});
}
+ /* If the supplied SqlNode when fully qualified is in the set of
bypassQualifieds, then we
+ remove all entries in the qualifieds set as well as remnantMustFilterFields
+ that belong to the same table identifier.
+ */
+ private static void purgeForBypassFields(SqlNode node, SqlValidatorScope
scope,
+ Set<SqlQualified> qualifieds, Set<SqlQualified> bypassQualifieds,
+ Set<SqlQualified> remnantMustFilterFields) {
+ node.accept(new SqlBasicVisitor<Void>() {
+ @Override public Void visit(SqlIdentifier id) {
+ final SqlQualified qualified = scope.fullyQualify(id);
+ if (bypassQualifieds.contains(qualified)) {
+ // Clear all the must-filter qualifieds from the same table
identifier
+ Collection<SqlQualified> sameIdentifier = qualifieds.stream()
+ .filter(q -> qualifiedMatchesIdentifier(q, qualified))
+ .collect(Collectors.toList());
+ sameIdentifier.forEach(qualifieds::remove);
+
+ // Clear all the remnant must-filter qualifieds from the same table
identifier
+ Collection<SqlQualified> sameIdentifier_ =
remnantMustFilterFields.stream()
+ .filter(q -> qualifiedMatchesIdentifier(q, qualified))
+ .collect(Collectors.toList());
+ sameIdentifier_.forEach(remnantMustFilterFields::remove);
+ }
+ return null;
+ }
+ });
+ }
+
+ private static void toQualifieds(ImmutableBitSet fields, Set<SqlQualified>
qualifiedSet,
+ SelectScope fromScope, ScopeChild child, List<String> fieldNames) {
+ fields.forEachInt(i ->
+ qualifiedSet.add(
+ SqlQualified.create(fromScope, 1, child.namespace,
+ new SqlIdentifier(
+ ImmutableList.of(child.name, fieldNames.get(i)),
+ SqlParserPos.ZERO))));
+
+ }
+ private static boolean qualifiedMatchesIdentifier(SqlQualified q1,
SqlQualified q2) {
Review Comment:
add blank line before
##########
core/src/main/java/org/apache/calcite/sql/validate/AbstractNamespace.java:
##########
@@ -62,6 +64,10 @@ abstract class AbstractNamespace implements
SqlValidatorNamespace {
* should typically be re-assigned on validate. */
protected ImmutableBitSet mustFilterFields = ImmutableBitSet.of();
+ protected ImmutableBitSet mustFilterBypassFields = ImmutableBitSet.of();
Review Comment:
It seems that `mustFilterFields`, `mustFilterBypassFields`,
`remnantMustFilterFields` are always set at the same time. Would it simplify to
combine into a single class?
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -1217,12 +1219,21 @@ protected void validateNamespace(final
SqlValidatorNamespace namespace,
// fields; these are neutralized if the consuming query filters on
them.
final ImmutableBitSet mustFilterFields =
namespace.getMustFilterFields();
- if (!mustFilterFields.isEmpty()) {
+ // Remnant must filter fields are fields that are not selected and
cannot
+ // be defused unlessa bypass field defuses it.
+ // A top-level namespace must not have any remnant-must-filter fields.
+ final Set<SqlQualified> remnantMustFilterFields =
namespace.getRemnantMustFilterFields();
+ if (!mustFilterFields.isEmpty() || !remnantMustFilterFields.isEmpty())
{
+ Stream<String> mustFilterStream =
+ StreamSupport.stream(mustFilterFields.spliterator(), false)
Review Comment:
should be indent 4 not indent 8
##########
testkit/src/main/java/org/apache/calcite/test/catalog/MockCatalogReader.java:
##########
@@ -1026,27 +1028,32 @@ public static class MockDynamicTable
public static class MustFilterMockTable
extends MockTable implements SemanticTable {
private final Map<String, String> fieldFilters;
+ private final List<Integer> bypassFieldList;
+
+ private final Set<SqlQualified> remnantFieldFilters;
MustFilterMockTable(MockCatalogReader catalogReader, String catalogName,
String schemaName, String name, boolean stream, boolean temporal,
double rowCount, @Nullable ColumnResolver resolver,
InitializerExpressionFactory initializerExpressionFactory,
- Map<String, String> fieldFilters) {
+ Map<String, String> fieldFilters, List<Integer> bypassFieldList) {
super(catalogReader, catalogName, schemaName, name, stream, temporal,
rowCount, resolver, initializerExpressionFactory);
this.fieldFilters = ImmutableMap.copyOf(fieldFilters);
+ this.bypassFieldList = ImmutableList.copyOf(bypassFieldList);
+ this.remnantFieldFilters = Collections.emptySet();
Review Comment:
Use `ImmutableSet.of()` rather than `Collections.emptySet()` (here and
elsewhere in your PR)
--
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]
