Skip to content
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.

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

Optimize exec async #16

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*
* Copyright 2022 RelationalAI, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"): you may
* not use this file except in compliance with the License. You may obtain
* a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package com.relationalai.examples;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;

import com.relationalai.Client;
import com.relationalai.Config;
import com.relationalai.HttpError;
import com.relationalai.Json;

public class Execute implements Runnable {
boolean readonly;
String database, engine, command, filename, profile;

// Returns the name of the file, without extension.
static String readFile(String fname) throws IOException {
return Files.readAllBytes(Path.of(fname)).toString();
}

String getCommand() throws IOException {
if (command != null)
return command; // prefer command line
if (filename != null)
return readFile(filename);
return null;
}

public void parseArgs(String[] args) {
var c = Command.create("Execute")
.addArgument("database")
.addArgument("engine")
.addOption("c", "rel source string")
.addOption("f", "rel source file")
.addFlag("readonly", "readonly query (default: false)")
.parseArgs(args);
this.database = c.getValue("database");
this.engine = c.getValue("engine");
this.command = c.getValue("c");
this.filename = c.getValue("f");
this.readonly = c.getValue("readonly", Boolean.class);
this.profile = c.getValue("profile");
}

public void run(String[] args) throws HttpError, InterruptedException, IOException {
parseArgs(args);
var cfg = Config.loadConfig("~/.rai/config", profile);
var client = new Client(cfg);
String source = getCommand();
if (source == null)
return; // nothing to execute
var rsp = client.execute(database, engine, source, readonly);
Json.print(rsp);
}
}
28 changes: 10 additions & 18 deletions rai-sdk/src/main/java/com/relationalai/Client.java
Original file line number Diff line number Diff line change
Expand Up @@ -705,25 +705,22 @@ public TransactionAsyncResult execute(
String source, boolean readonly,
Map<String, String> inputs) throws HttpError, IOException, InterruptedException {

var id = executeAsync(database, engine, source, readonly, inputs).transaction.id;
var rsp = executeAsync(database, engine, source, readonly, inputs);

var transaction = getTransaction(id).transaction;
if (rsp.gotCompletedResult)
return rsp;

var transaction = getTransaction(rsp.transaction.id).transaction;
while ( !("COMPLETED".equals(transaction.state) || "ABORTED".equals(transaction.state)) ) {
vilterp marked this conversation as resolved.
Show resolved Hide resolved
Thread.sleep(2000);
transaction = getTransaction(id).transaction;
transaction = getTransaction(transaction.id).transaction;
}

var results = getTransactionResults(id);
var metadata = getTransactionMetadata(id);
var problems = getTransactionProblems(id);
var results = getTransactionResults(transaction.id);
var metadata = getTransactionMetadata(transaction.id);
var problems = getTransactionProblems(transaction.id);

return new TransactionAsyncResult(
transaction,
results,
metadata,
problems
);
return new TransactionAsyncResult(transaction, results, metadata, problems, true);
}

public TransactionAsyncResult executeAsync(
Expand Down Expand Up @@ -775,12 +772,7 @@ private TransactionAsyncResult readTransactionAsyncResults(List<TransactionAsync
var problemsResult = parseProblemsResult(new String(problems.get(0).data, StandardCharsets.UTF_8));

var results = readArrowFiles(files);
return new TransactionAsyncResult(
transactionResponse,
results,
Arrays.asList(metadataResponse),
problemsResult
);
return new TransactionAsyncResult(transactionResponse, results, Arrays.asList(metadataResponse), problemsResult, true);
}

public TransactionAsyncSingleResponse getTransaction(String id) throws HttpError, IOException, InterruptedException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,32 @@

public class TransactionAsyncResult extends Entity {

public Boolean gotCompletedResult;
Copy link

Choose a reason for hiding this comment

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

let's call this gotCompleteResult, not gotCompletedResult, inline with the Go SDK: https://github.com/RelationalAI/rai-sdk-go/blob/bbc1c3bba86b43865e983d992d48161e5652530f/rai/models.go#L302-L303

It doesn't mean that the transaction was completed, it means that it was either completed or aborted, and we got the results.

Copy link

Choose a reason for hiding this comment

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

I guess there are a few cases we're covering with this type:

  1. ExecuteAsync => fast path; contains results. state is aborted or completed
  2. ExecuteAsync => slow path; doesn't contain results. state is running
  3. Execute => regardless of path, contains results. either aborted or completed

I guess gotCompleteResult differentiates (1) from (2)? But why do we need it? Can't we use the state to differentiate?

What am I missing @NHDaly

Copy link
Member

@NHDaly NHDaly Aug 11, 2022

Choose a reason for hiding this comment

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

We discussed how in the original spec (before you joined the project, i think, @vilterp), we intended to have a fourth state:
4. ExecuteAsync => fast path; does not contain results (because they are too big to send over the wire, and you presumably want to page them). state is aborted or completed

So i think this isn't strictly needed right now; it's a future-proofing that you and I did while we were looking at the code.

Copy link

Choose a reason for hiding this comment

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

K thanks. I forgot that it was future proofing and not a case we need now.

public TransactionAsyncCompactResponse transaction;
public List<ArrowRelation> results;
public List<TransactionAsyncMetadataResponse> metadata;
public List<Object> problems;

public TransactionAsyncResult(TransactionAsyncCompactResponse transaction, List<ArrowRelation> results, List<TransactionAsyncMetadataResponse> metadata, List<Object> problems) {
public TransactionAsyncResult(
TransactionAsyncCompactResponse transaction,
List<ArrowRelation> results,
List<TransactionAsyncMetadataResponse> metadata,
List<Object> problems
) {
this.transaction = transaction;
this.results = results;
this.metadata = metadata;
this.problems = problems;
this.gotCompletedResult = false;
}
public TransactionAsyncResult(
TransactionAsyncCompactResponse transaction,
List<ArrowRelation> results,
List<TransactionAsyncMetadataResponse> metadata,
List<Object> problems,
Boolean gotCompletedResult
) {
this(transaction, results, metadata, problems);
this.gotCompletedResult = false;
NRHelmi marked this conversation as resolved.
Show resolved Hide resolved
}
}