Skip to content

Added missing unittests for primitive schema types#4147

Merged
srkukarni merged 5 commits intoapache:masterfrom
srkukarni:add_schema_info
Apr 27, 2019
Merged

Added missing unittests for primitive schema types#4147
srkukarni merged 5 commits intoapache:masterfrom
srkukarni:add_schema_info

Conversation

@srkukarni
Copy link
Contributor

@srkukarni srkukarni commented Apr 26, 2019

Motivation

Currently we dont have any unittests for schemas. This pr adds them

Modifications

Describe the modifications you've done.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@srkukarni srkukarni added this to the 2.4.0 milestone Apr 26, 2019
@srkukarni srkukarni requested review from jerrypeng and sijie April 26, 2019 22:26
@srkukarni srkukarni self-assigned this Apr 26, 2019
SchemaCompatibilityStrategy strategy) {
if (SchemaType.isPrimitiveSchemaType(existingSchema.schema.getType())) {
// for primitive data types, only schema type check is needed
return existingSchema.schema.getType() == newSchema.getType();
Copy link
Contributor

@jerrypeng jerrypeng Apr 26, 2019

Choose a reason for hiding this comment

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

@srkukarni even from primitive types you still should check this condition:

compatibilityChecks.getOrDefault(newSchema.getType(), SchemaCompatibilityCheck.DEFAULT)
            .isCompatible(existingSchema.schema, newSchema, strategy);	            

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jerrypeng fixed

Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

I don't think we should add avro schema info to primitive schema types. moving my -1 from #4022 here.

  1. SchemaType is already enough to describe what is the primitive type and how it is serialized and deserialized. Adding avro schema data to the schema info is redundant. As I explained in #4022, you don't need additional avro schema data for supporting primitive types in presto (or integrating with other query/computing engines).

  2. adding schema data is a breaking change. it is breaking at two levels:

a) primitive types are not evolvable. so you shouldn't change schema data and schema type. unless there is a really really strong reason to break this assumption.
b) for supporting a query engine to query primitive types, you don't need to add schema data. schema type is enough. it seems to me that you are taking a wrong approach on implementing a feature that doesn't require adding redundant schema data, which enforcing people to upgrade to break their applications.

@srkukarni
Copy link
Contributor Author

@sijie @jerrypeng Based on @sijie feedback and our discussion, I've removed the schema changes and only kept the unittest additions. Please take a look again. Thanks!

@srkukarni srkukarni changed the title Added schema information and unittests for primitive schema types Added missing unittests for primitive schema types Apr 27, 2019
Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

Just FYI all primitive schemas are already unit tested by PrimitiveSchemaTest. But it is always good to have more tests.

@srkukarni
Copy link
Contributor Author

run cpp tests
run java8 tests

@srkukarni srkukarni merged commit d1d5aba into apache:master Apr 27, 2019
@srkukarni srkukarni deleted the add_schema_info branch April 27, 2019 16:00
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.

3 participants