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

test: serialize/deserialize plans from qtt #4080

Merged
merged 1 commit into from
Dec 10, 2019

Conversation

rodesai
Copy link
Contributor

@rodesai rodesai commented Dec 7, 2019

Extends qtt to build the query plan and deserialize/serialize it
before executing. Should be enough to make sure plans are serializable
and executable until we improve qtt.

@rodesai rodesai requested a review from a team as a code owner December 7, 2019 22:04
@@ -43,12 +48,6 @@ private QueryContext(List<String> context) {
}
}

@SuppressWarnings("unused")// Invoked via reflection by Jackson
@JsonCreator
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason Jackson complains it can't deserialize into ImmutableList when using the JsonCreator (even though it ultimately just calls this constructor). So I switched this class to use a custom deserializer.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's because there is a getter called getContext, so its checking out the private final ImmutableList<String> context; field. Mark getContext as @JsonIgnore and you'll be golden.

@@ -25,6 +25,11 @@
import java.io.IOException;

final class LogicalSchemaDeserializer extends JsonDeserializer<LogicalSchema> {
final boolean withImplicitColumns;

LogicalSchemaDeserializer(final boolean withImplicitColumns) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this because when deserializing ddl schemas we do want the implicit columns.

Copy link
Contributor

Choose a reason for hiding this comment

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

Out of interest, is it just ROWTIME you need? I'm hoping to completely remove the concept of 'implicit' column soon by:

  1. making ROWTIME a pseudo column
  2. explicitly adding ROWTIME in CT/CS statements if a key column is not provided.

Copy link
Contributor Author

@rodesai rodesai Dec 10, 2019

Choose a reason for hiding this comment

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

explicitly adding ROWTIME in CT/CS statements if a key column is not provided

you mean ROWKEY?

Copy link
Contributor

@big-andy-coates big-andy-coates left a comment

Choose a reason for hiding this comment

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

Thanks @rodesai, LGTM

@@ -43,12 +48,6 @@ private QueryContext(List<String> context) {
}
}

@SuppressWarnings("unused")// Invoked via reflection by Jackson
@JsonCreator
Copy link
Contributor

Choose a reason for hiding this comment

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

It's because there is a getter called getContext, so its checking out the private final ImmutableList<String> context; field. Mark getContext as @JsonIgnore and you'll be golden.

@@ -342,6 +348,24 @@ private static ExecuteResultAndSortedSources execute(
Optional.empty());
}

@SuppressWarnings("unchecked")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: don't think this suppression is needed

Comment on lines 352 to 361
private static ExecuteResult executeConfiguredStatement(
final KsqlExecutionContext executionContext,
final ConfiguredStatement<?> stmt) {
final KsqlPlan plan = executionContext.plan(executionContext.getServiceContext(), stmt);
final ConfiguredKsqlPlan configuredPlan;
try {
final String serialized = PLAN_MAPPER.writeValueAsString(plan);
configuredPlan = ConfiguredKsqlPlan.of(
PLAN_MAPPER.readValue(serialized, KsqlPlan.class),
stmt.getOverrides(),
stmt.getConfig());
} catch (final IOException e) {
throw new RuntimeException(e);
}
return executionContext.execute(executionContext.getServiceContext(), configuredPlan);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a style thing, but I generally think that if you've got some part of the code in a function that wants its own try/catch, it generally makes the code more readable to move it to its own function, e.g.

  private static ExecuteResult executeConfiguredStatement(
      final KsqlExecutionContext executionContext,
      final ConfiguredStatement<?> stmt
  ) {
    final KsqlPlan plan = executionContext.plan(executionContext.getServiceContext(), stmt);
    final ConfiguredKsqlPlan configuredPlan = buildConfiguredKsqlPlan(stmt, plan);
    return executionContext.execute(executionContext.getServiceContext(), configuredPlan);
  }

  private static ConfiguredKsqlPlan buildConfiguredKsqlPlan(
      final ConfiguredStatement<?> stmt,
      final KsqlPlan plan
  ) {
    try {
      final String serialized = PLAN_MAPPER.writeValueAsString(plan);
      return ConfiguredKsqlPlan.of(
          PLAN_MAPPER.readValue(serialized, KsqlPlan.class),
          stmt.getOverrides(),
          stmt.getConfig()
      );
    } catch (final IOException e) {
      throw new RuntimeException(e);
    }
  }

stmt.getOverrides(),
stmt.getConfig());
} catch (final IOException e) {
throw new RuntimeException(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe throw a TestFrameworkException?

@@ -25,6 +25,11 @@
import java.io.IOException;

final class LogicalSchemaDeserializer extends JsonDeserializer<LogicalSchema> {
final boolean withImplicitColumns;

LogicalSchemaDeserializer(final boolean withImplicitColumns) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of interest, is it just ROWTIME you need? I'm hoping to completely remove the concept of 'implicit' column soon by:

  1. making ROWTIME a pseudo column
  2. explicitly adding ROWTIME in CT/CS statements if a key column is not provided.

Extends qtt to build the query plan and deserialize/serialize it
before executing. Should be enough to make sure plans are serializable
and executable until we improve qtt.
@rodesai rodesai force-pushed the serialize-deserialize-from-qtt branch from 34bb53c to fd78b0c Compare December 10, 2019 19:58
@rodesai rodesai merged commit 4dd76ac into confluentinc:master Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants