Skip to content

Small code cleanup #10690

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
findepi merged 4 commits into from
Jan 19, 2022
Merged

Small code cleanup #10690

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e296705
Fix typo
findepi Jan 19, 2022
edda432
Remove ignored methor return value
findepi Jan 19, 2022
799a21f
Remove redundant code
findepi Jan 19, 2022
ed0add0
Remove unused method in SpatialJoinUtils
findepi Jan 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 0 additions & 65 deletions core/trino-main/src/main/java/io/trino/util/SpatialJoinUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,25 +13,15 @@
*/
package io.trino.util;

import io.trino.sql.planner.Symbol;
import io.trino.sql.planner.plan.PlanNode;
import io.trino.sql.tree.ComparisonExpression;
import io.trino.sql.tree.Expression;
import io.trino.sql.tree.FunctionCall;
import io.trino.sql.tree.Literal;
import io.trino.sql.tree.SymbolReference;

import java.util.Collection;
import java.util.List;
import java.util.Set;

import static com.google.common.base.Verify.verify;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static io.trino.metadata.ResolvedFunction.extractFunctionName;
import static io.trino.sql.ExpressionUtils.extractConjuncts;
import static io.trino.sql.tree.ComparisonExpression.Operator.LESS_THAN;
import static io.trino.sql.tree.ComparisonExpression.Operator.LESS_THAN_OR_EQUAL;

public final class SpatialJoinUtils
{
Expand Down Expand Up @@ -108,59 +98,4 @@ private static boolean isSTDistance(Expression expression)

return false;
}

public static boolean isSpatialJoinFilter(PlanNode left, PlanNode right, Expression filterExpression)
{
List<FunctionCall> functionCalls = extractSupportedSpatialFunctions(filterExpression);
for (FunctionCall functionCall : functionCalls) {
if (isSpatialJoinFilter(left, right, functionCall)) {
return true;
}
}

List<ComparisonExpression> spatialComparisons = extractSupportedSpatialComparisons(filterExpression);
for (ComparisonExpression spatialComparison : spatialComparisons) {
if (spatialComparison.getOperator() == LESS_THAN || spatialComparison.getOperator() == LESS_THAN_OR_EQUAL) {
// ST_Distance(a, b) <= r
Expression radius = spatialComparison.getRight();
if (radius instanceof Literal || (radius instanceof SymbolReference && getSymbolReferences(right.getOutputSymbols()).contains(radius))) {
if (isSpatialJoinFilter(left, right, (FunctionCall) spatialComparison.getLeft())) {
return true;
}
}
}
}

return false;
}

private static boolean isSpatialJoinFilter(PlanNode left, PlanNode right, FunctionCall spatialFunction)
{
List<Expression> arguments = spatialFunction.getArguments();
verify(arguments.size() == 2);
if (!(arguments.get(0) instanceof SymbolReference) || !(arguments.get(1) instanceof SymbolReference)) {
return false;
}

SymbolReference firstSymbol = (SymbolReference) arguments.get(0);
SymbolReference secondSymbol = (SymbolReference) arguments.get(1);

Set<SymbolReference> probeSymbols = getSymbolReferences(left.getOutputSymbols());
Set<SymbolReference> buildSymbols = getSymbolReferences(right.getOutputSymbols());

if (probeSymbols.contains(firstSymbol) && buildSymbols.contains(secondSymbol)) {
return true;
}

if (probeSymbols.contains(secondSymbol) && buildSymbols.contains(firstSymbol)) {
return true;
}

return false;
}

private static Set<SymbolReference> getSymbolReferences(Collection<Symbol> symbols)
{
return symbols.stream().map(Symbol::toSymbolReference).collect(toImmutableSet());
}
}
2 changes: 1 addition & 1 deletion plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryClientFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public BigQueryClientFactory(
ViewMaterializationCache materializationCache,
HeaderProvider headerProvider)
{
this.identityCacheMapping = requireNonNull(identityCacheMapping, "identityCacheMaping is null");
this.identityCacheMapping = requireNonNull(identityCacheMapping, "identityCacheMapping is null");
this.credentialsSupplier = requireNonNull(credentialsSupplier, "credentialsSupplier is null");
this.bigQueryConfig = requireNonNull(bigQueryConfig, "bigQueryConfig is null");
this.materializationCache = requireNonNull(materializationCache, "materializationCache is null");
Expand Down
4 changes: 0 additions & 4 deletions plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,10 +494,6 @@ private List<String> getPinotTableNames()

private static <K, V> V getFromCache(LoadingCache<K, V> cache, K key)
{
V value = cache.getIfPresent(key);
Copy link
Copy Markdown
Member

@hashhar hashhar Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC there was some mention that getIfPresent is more contention safe than get? Or was it about computeIfAbsent?

I'll check and get back.

Copy link
Copy Markdown
Member

@hashhar hashhar Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However this doesn't seem to be in a hot path so shouldn't matter. Readability first.

Copy link
Copy Markdown
Member Author

@findepi findepi Jan 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hashhar thanks for reminding me.
yes, i think it doesn't matter here
i think this is the thread: #9980

cc @sopel39

if (value != null) {
return value;
}
try {
return cache.get(key);
}
Expand Down
3 changes: 1 addition & 2 deletions plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,14 +140,13 @@ public PinotClient(
.build((CacheLoader.from(this::getAllBrokersForTable)));
}

public static JsonCodecBinder addJsonBinders(JsonCodecBinder jsonCodecBinder)
public static void addJsonBinders(JsonCodecBinder jsonCodecBinder)
{
jsonCodecBinder.bindJsonCodec(GetTables.class);
jsonCodecBinder.bindJsonCodec(BrokersForTable.InstancesInBroker.class);
jsonCodecBinder.bindJsonCodec(BrokersForTable.class);
jsonCodecBinder.bindJsonCodec(TimeBoundary.class);
jsonCodecBinder.bindJsonCodec(BrokerResponseNative.class);
return jsonCodecBinder;
}

protected <T> T doHttpActionWithHeadersJson(Request.Builder requestBuilder, Optional<String> requestBody, JsonCodec<T> codec)
Expand Down