Skip to content

Leverage Optional.orElseThrow to increase code readability #13941

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 2 commits into from
Sep 5, 2022
Merged

Leverage Optional.orElseThrow to increase code readability #13941

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff2ae09
Leverage Optional.orElseThrow to increase code readability
findepi Aug 31, 2022
604c4e3
Remove redundant else
findepi Sep 5, 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
2 changes: 1 addition & 1 deletion client/trino-cli/src/main/java/io/trino/cli/OutputHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public void processRows(StatementClient client)
if (row == END_TOKEN) {
break;
}
else if (row != null) {
if (row != null) {
rowBuffer.add(row);
}
}
Expand Down
10 changes: 4 additions & 6 deletions core/trino-main/src/main/java/io/trino/cost/CostCalculatorWithEstimatedExchanges.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,12 +280,10 @@ private static LocalCostEstimate calculateJoinExchangeCost(
LocalCostEstimate localRepartitionCost = calculateLocalRepartitionCost(buildSizeInBytes);
return addPartialComponents(replicateCost, localRepartitionCost);
}
else {
LocalCostEstimate probeCost = calculateRemoteRepartitionCost(probeSizeInBytes);
LocalCostEstimate buildRemoteRepartitionCost = calculateRemoteRepartitionCost(buildSizeInBytes);
LocalCostEstimate buildLocalRepartitionCost = calculateLocalRepartitionCost(buildSizeInBytes);
return addPartialComponents(probeCost, buildRemoteRepartitionCost, buildLocalRepartitionCost);
}
LocalCostEstimate probeCost = calculateRemoteRepartitionCost(probeSizeInBytes);
LocalCostEstimate buildRemoteRepartitionCost = calculateRemoteRepartitionCost(buildSizeInBytes);
LocalCostEstimate buildLocalRepartitionCost = calculateLocalRepartitionCost(buildSizeInBytes);
return addPartialComponents(probeCost, buildRemoteRepartitionCost, buildLocalRepartitionCost);
}

public static LocalCostEstimate calculateJoinInputCost(
Expand Down
22 changes: 10 additions & 12 deletions core/trino-main/src/main/java/io/trino/cost/ScalarStatsCalculator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,20 +312,18 @@ private SymbolStatsEstimate estimateCoalesce(SymbolStatsEstimate left, SymbolSta
if (left.getNullsFraction() == 0) {
return left;
}
else if (left.getNullsFraction() == 1.0) {
if (left.getNullsFraction() == 1.0) {
return right;
}
else {
return SymbolStatsEstimate.builder()
.setLowValue(min(left.getLowValue(), right.getLowValue()))
.setHighValue(max(left.getHighValue(), right.getHighValue()))
.setDistinctValuesCount(left.getDistinctValuesCount() +
min(right.getDistinctValuesCount(), input.getOutputRowCount() * left.getNullsFraction()))
.setNullsFraction(left.getNullsFraction() * right.getNullsFraction())
// TODO check if dataSize estimation method is correct
.setAverageRowSize(max(left.getAverageRowSize(), right.getAverageRowSize()))
.build();
}
return SymbolStatsEstimate.builder()
.setLowValue(min(left.getLowValue(), right.getLowValue()))
.setHighValue(max(left.getHighValue(), right.getHighValue()))
.setDistinctValuesCount(left.getDistinctValuesCount() +
min(right.getDistinctValuesCount(), input.getOutputRowCount() * left.getNullsFraction()))
.setNullsFraction(left.getNullsFraction() * right.getNullsFraction())
// TODO check if dataSize estimation method is correct
.setAverageRowSize(max(left.getAverageRowSize(), right.getAverageRowSize()))
.build();
}
}

Expand Down
14 changes: 6 additions & 8 deletions core/trino-main/src/main/java/io/trino/dispatcher/QueuedStatementResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,15 +391,13 @@ public QueryResults getQueryResults(long token, UriInfo uriInfo)
DispatchInfo.queued(NO_DURATION, NO_DURATION));
}

Optional<DispatchInfo> dispatchInfo = dispatchManager.getDispatchInfo(queryId);
if (dispatchInfo.isEmpty()) {
// query should always be found, but it may have just been determined to be abandoned
throw new WebApplicationException(Response
.status(NOT_FOUND)
.build());
}
DispatchInfo dispatchInfo = dispatchManager.getDispatchInfo(queryId)
// query should always be found, but it may have just been determined to be abandoned
.orElseThrow(() -> new WebApplicationException(Response
.status(NOT_FOUND)
.build()));

return createQueryResults(token + 1, uriInfo, dispatchInfo.get());
return createQueryResults(token + 1, uriInfo, dispatchInfo);
}

public void cancel()
Expand Down
9 changes: 3 additions & 6 deletions core/trino-main/src/main/java/io/trino/execution/CommentTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@

import java.util.List;
import java.util.Map;
import java.util.Optional;

import static com.google.common.util.concurrent.Futures.immediateVoidFuture;
import static io.trino.metadata.MetadataUtil.createQualifiedObjectName;
Expand Down Expand Up @@ -133,12 +132,10 @@ else if (metadata.getTableHandle(session, viewName).isPresent()) {

private void commentOnColumn(Comment statement, Session session)
{
Optional<QualifiedName> prefix = statement.getName().getPrefix();
if (prefix.isEmpty()) {
throw semanticException(MISSING_TABLE, statement, "Table must be specified");
}
QualifiedName prefix = statement.getName().getPrefix()
.orElseThrow(() -> semanticException(MISSING_TABLE, statement, "Table must be specified"));

QualifiedObjectName originalTableName = createQualifiedObjectName(session, statement, prefix.get());
QualifiedObjectName originalTableName = createQualifiedObjectName(session, statement, prefix);
RedirectionAwareTableHandle redirectionAwareTableHandle = metadata.getRedirectionAwareTableHandle(session, originalTableName);
if (redirectionAwareTableHandle.getTableHandle().isEmpty()) {
throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: " + originalTableName);
Expand Down
5 changes: 1 addition & 4 deletions core/trino-main/src/main/java/io/trino/execution/CommitTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,7 @@ public ListenableFuture<Void> execute(
WarningCollector warningCollector)
{
Session session = stateMachine.getSession();
if (session.getTransactionId().isEmpty()) {
throw new TrinoException(NOT_IN_TRANSACTION, "No transaction in progress");
}
TransactionId transactionId = session.getTransactionId().get();
TransactionId transactionId = session.getTransactionId().orElseThrow(() -> new TrinoException(NOT_IN_TRANSACTION, "No transaction in progress"));

stateMachine.clearTransactionId();
return transactionManager.asyncCommit(transactionId);
Expand Down
4 changes: 1 addition & 3 deletions core/trino-main/src/main/java/io/trino/execution/DenyTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import io.trino.metadata.Metadata;
import io.trino.metadata.QualifiedObjectName;
import io.trino.metadata.RedirectionAwareTableHandle;
import io.trino.metadata.TableHandle;
import io.trino.security.AccessControl;
import io.trino.spi.connector.CatalogSchemaName;
import io.trino.spi.security.Privilege;
Expand Down Expand Up @@ -97,8 +96,7 @@ private static void executeDenyOnTable(Session session, Deny statement, Metadata
{
QualifiedObjectName tableName = createQualifiedObjectName(session, statement, statement.getName());
RedirectionAwareTableHandle redirection = metadata.getRedirectionAwareTableHandle(session, tableName);
Optional<TableHandle> tableHandle = redirection.getTableHandle();
if (tableHandle.isEmpty()) {
if (redirection.getTableHandle().isEmpty()) {
Copy link
Copy Markdown
Member

@ebyhr ebyhr Aug 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I feel this change unrelates to Optional.orElseThrow leverage. Is separated a commit overkill?

Copy link
Copy Markdown
Member Author

@findepi findepi Sep 2, 2022

Choose a reason for hiding this comment

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

it's related.

The pattern

Optional<...> x = expr;
if (x.isEmpty()) {
  throw;
}

should generally be avoided and you should use orElseThrow.
You should not, however, use orElseThrow where the variable would become unused. I.e. you shouldn't use it, if your goal is to check existence of some optional value. In such cases, i inlined the expression

throw semanticException(TABLE_NOT_FOUND, statement, "Table '%s' does not exist", tableName);
}
if (redirection.getRedirectedTableName().isPresent()) {
Expand Down
4 changes: 1 addition & 3 deletions core/trino-main/src/main/java/io/trino/execution/GrantTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import io.trino.metadata.Metadata;
import io.trino.metadata.QualifiedObjectName;
import io.trino.metadata.RedirectionAwareTableHandle;
import io.trino.metadata.TableHandle;
import io.trino.security.AccessControl;
import io.trino.spi.connector.CatalogSchemaName;
import io.trino.spi.security.Privilege;
Expand Down Expand Up @@ -101,8 +100,7 @@ private void executeGrantOnTable(Session session, Grant statement)
{
QualifiedObjectName tableName = createQualifiedObjectName(session, statement, statement.getName());
RedirectionAwareTableHandle redirection = metadata.getRedirectionAwareTableHandle(session, tableName);
Optional<TableHandle> tableHandle = redirection.getTableHandle();
if (tableHandle.isEmpty()) {
if (redirection.getTableHandle().isEmpty()) {
throw semanticException(TABLE_NOT_FOUND, statement, "Table '%s' does not exist", tableName);
}
if (redirection.getRedirectedTableName().isPresent()) {
Expand Down
4 changes: 1 addition & 3 deletions core/trino-main/src/main/java/io/trino/execution/RevokeTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import io.trino.metadata.Metadata;
import io.trino.metadata.QualifiedObjectName;
import io.trino.metadata.RedirectionAwareTableHandle;
import io.trino.metadata.TableHandle;
import io.trino.security.AccessControl;
import io.trino.spi.connector.CatalogSchemaName;
import io.trino.spi.security.Privilege;
Expand Down Expand Up @@ -101,8 +100,7 @@ private void executeRevokeOnTable(Session session, Revoke statement)
{
QualifiedObjectName tableName = createQualifiedObjectName(session, statement, statement.getName());
RedirectionAwareTableHandle redirection = metadata.getRedirectionAwareTableHandle(session, tableName);
Optional<TableHandle> tableHandle = redirection.getTableHandle();
if (tableHandle.isEmpty()) {
if (redirection.getTableHandle().isEmpty()) {
throw semanticException(TABLE_NOT_FOUND, statement, "Table '%s' does not exist", tableName);
}
if (redirection.getRedirectedTableName().isPresent()) {
Expand Down
5 changes: 1 addition & 4 deletions core/trino-main/src/main/java/io/trino/execution/RollbackTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,7 @@ public ListenableFuture<Void> execute(
WarningCollector warningCollector)
{
Session session = stateMachine.getSession();
if (session.getTransactionId().isEmpty()) {
throw new TrinoException(NOT_IN_TRANSACTION, "No transaction in progress");
}
TransactionId transactionId = session.getTransactionId().get();
TransactionId transactionId = session.getTransactionId().orElseThrow(() -> new TrinoException(NOT_IN_TRANSACTION, "No transaction in progress"));

stateMachine.clearTransactionId();
transactionManager.asyncAbort(transactionId);
Expand Down
8 changes: 3 additions & 5 deletions core/trino-main/src/main/java/io/trino/execution/SetPropertiesTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,13 +118,11 @@ private void setTableProperties(SetProperties statement, QualifiedObjectName tab
throw semanticException(NOT_SUPPORTED, statement, "Cannot set properties to a view in ALTER TABLE");
}

Optional<TableHandle> tableHandle = plannerContext.getMetadata().getTableHandle(session, tableName);
if (tableHandle.isEmpty()) {
throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: %s", tableName);
}
TableHandle tableHandle = plannerContext.getMetadata().getTableHandle(session, tableName)
.orElseThrow(() -> semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: %s", tableName));

accessControl.checkCanSetTableProperties(session.toSecurityContext(), tableName, properties);
plannerContext.getMetadata().setTableProperties(session, tableHandle.get(), properties);
plannerContext.getMetadata().setTableProperties(session, tableHandle, properties);
}

private void setMaterializedViewProperties(
Expand Down
4 changes: 1 addition & 3 deletions core/trino-main/src/main/java/io/trino/execution/SetTableAuthorizationTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import io.trino.metadata.Metadata;
import io.trino.metadata.QualifiedObjectName;
import io.trino.metadata.RedirectionAwareTableHandle;
import io.trino.metadata.TableHandle;
import io.trino.security.AccessControl;
import io.trino.spi.security.TrinoPrincipal;
import io.trino.sql.tree.Expression;
Expand Down Expand Up @@ -71,8 +70,7 @@ public ListenableFuture<Void> execute(

getRequiredCatalogHandle(metadata, session, statement, tableName.getCatalogName());
RedirectionAwareTableHandle redirection = metadata.getRedirectionAwareTableHandle(session, tableName);
Optional<TableHandle> tableHandle = redirection.getTableHandle();
if (tableHandle.isEmpty()) {
if (redirection.getTableHandle().isEmpty()) {
throw semanticException(TABLE_NOT_FOUND, statement, "Table '%s' does not exist", tableName);
}
if (redirection.getRedirectedTableName().isPresent()) {
Expand Down
6 changes: 2 additions & 4 deletions core/trino-main/src/main/java/io/trino/execution/SplitAssignment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,8 @@ public SplitAssignment update(SplitAssignment assignment)
newSplits,
assignment.isNoMoreSplits());
}
else {
// the specified assignment is older than this one
return this;
}
// the specified assignment is older than this one
return this;
}

private boolean isNewer(SplitAssignment assignment)
Expand Down
9 changes: 3 additions & 6 deletions core/trino-main/src/main/java/io/trino/execution/TruncateTableTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import javax.inject.Inject;

import java.util.List;
import java.util.Optional;

import static com.google.common.util.concurrent.Futures.immediateFuture;
import static io.trino.metadata.MetadataUtil.createQualifiedObjectName;
Expand Down Expand Up @@ -72,14 +71,12 @@ public ListenableFuture<Void> execute(
throw semanticException(NOT_SUPPORTED, statement, "Cannot truncate a view");
}

Optional<TableHandle> tableHandle = metadata.getTableHandle(session, tableName);
if (tableHandle.isEmpty()) {
throw semanticException(TABLE_NOT_FOUND, statement, "Table '%s' does not exist", tableName);
}
TableHandle tableHandle = metadata.getTableHandle(session, tableName)
.orElseThrow(() -> semanticException(TABLE_NOT_FOUND, statement, "Table '%s' does not exist", tableName));

accessControl.checkCanTruncateTable(session.toSecurityContext(), tableName);

metadata.truncateTable(session, tableHandle.get());
metadata.truncateTable(session, tableHandle);

return immediateFuture(null);
}
Expand Down
4 changes: 2 additions & 2 deletions core/trino-main/src/main/java/io/trino/execution/buffer/PagesSerdeUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ protected Page computeNext()
context.close(); // Release context buffers
return endOfData();
}
else if (read != headerBuffer.length) {
if (read != headerBuffer.length) {
throw new EOFException();
}

Expand Down Expand Up @@ -167,7 +167,7 @@ protected Slice computeNext()
if (read <= 0) {
return endOfData();
}
else if (read != headerBuffer.length) {
if (read != headerBuffer.length) {
throw new EOFException();
}

Expand Down
10 changes: 3 additions & 7 deletions core/trino-main/src/main/java/io/trino/execution/resourcegroups/InternalResourceGroup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,12 +234,10 @@ private ResourceGroupState getState()
if (canRunMore()) {
return CAN_RUN;
}
else if (canQueueMore()) {
if (canQueueMore()) {
return CAN_QUEUE;
}
else {
return FULL;
}
return FULL;
}
}

Expand Down Expand Up @@ -877,9 +875,7 @@ private static long getSubGroupSchedulingPriority(SchedulingPolicy policy, Inter
if (policy == QUERY_PRIORITY) {
return group.getHighestQueryPriority();
}
else {
return group.computeSchedulingWeight();
}
return group.computeSchedulingWeight();
}

private long computeSchedulingWeight()
Expand Down
4 changes: 1 addition & 3 deletions core/trino-main/src/main/java/io/trino/execution/resourcegroups/StochasticPriorityQueue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,9 +257,7 @@ public Node<E> addNode(E value, long tickets)
if (left.get().descendants < right.get().descendants) {
return left.get().addNode(value, tickets);
}
else {
return right.get().addNode(value, tickets);
}
return right.get().addNode(value, tickets);
}

Node<E> child = new Node<>(Optional.of(this), value);
Expand Down
6 changes: 2 additions & 4 deletions ...rino-main/src/main/java/io/trino/execution/scheduler/FixedSourcePartitionedScheduler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,10 +148,8 @@ public ScheduleResult schedule()
if (blockedReason != null) {
return new ScheduleResult(sourceSchedulers.isEmpty(), newTasks, blocked, blockedReason, splitsScheduled);
}
else {
checkState(blocked.isDone(), "blockedReason not provided when scheduler is blocked");
return new ScheduleResult(sourceSchedulers.isEmpty(), newTasks, splitsScheduled);
}
checkState(blocked.isDone(), "blockedReason not provided when scheduler is blocked");
return new ScheduleResult(sourceSchedulers.isEmpty(), newTasks, splitsScheduled);
}

@Override
Expand Down
Loading