Fix Variant Type

[lsetiawan]

Overview

This PR assigns Variant type to be Integers, based on https://github.com/ODM2/ODM2PythonAPI/blob/development/odm2api/ODM2/models.py#L11. Though datetime should serializes to datetime.

[emiliom]

Do you want me to review this tomorrow morning, before you merge, given what we discussed about SQLAlchemy Variant types?

[lsetiawan]

If you'd like, seem's like currently, Variant type is mapped to Integer based on ODM2API. I still couldn't figure out automatically determining that.

[emiliom]

I'll take a look tomorrow morning. Let's not merge yet.

[emiliom]

Can you point me to a couple of examples of specific odm2api model properties returned as variant type?

[lsetiawan]

Results

('ResultID', <class 'sqlalchemy.sql.type_api.Variant'>) # Variant HERE
('ResultUUID', <class 'sqlalchemy.sql.sqltypes.String'>)
('FeatureActionID', <class 'sqlalchemy.sql.sqltypes.Integer'>)
('ResultTypeCV', <class 'sqlalchemy.sql.sqltypes.String'>)
('VariableID', <class 'sqlalchemy.sql.sqltypes.Integer'>)
('UnitsID', <class 'sqlalchemy.sql.sqltypes.Integer'>)
('TaxonomicClassifierID', <class 'sqlalchemy.sql.sqltypes.Integer'>)
('ProcessingLevelID', <class 'sqlalchemy.sql.sqltypes.Integer'>)
('ResultDateTime', <class 'sqlalchemy.sql.sqltypes.DateTime'>)
('ResultDateTimeUTCOffset', <class 'sqlalchemy.sql.type_api.Variant'>)  # Variant HERE
('ValidDateTime', <class 'sqlalchemy.sql.sqltypes.DateTime'>)
('ValidDateTimeUTCOffset', <class 'sqlalchemy.sql.type_api.Variant'>)  # Variant HERE
('StatusCV', <class 'sqlalchemy.sql.sqltypes.String'>)
('SampledMediumCV', <class 'sqlalchemy.sql.sqltypes.String'>)
('ValueCount', <class 'sqlalchemy.sql.sqltypes.Integer'>)

[lsetiawan]

You can also look for BigIntegerType or DateTimeType within ODM2API.

BigIntegerType: https://github.com/ODM2/ODM2PythonAPI/blob/development/odm2api/ODM2/models.py#L395 DateTimeType: https://github.com/ODM2/ODM2PythonAPI/blob/development/odm2api/ODM2/models.py#L1656

[emiliom]

You can also look for BigIntegerType or DateTimeType within ODM2API.

Thanks. I had seen the BigIntegerType declaration, but somehow managed to miss the DateTimeType declaration right below it!

So, you're saying that Variant types only occur for these two types that are explicitly assigned to have variants? Furthermore, only BigIntegerType is assigned a variant for most RDBMS types; DateTimeType is currently assigned a variant only for SQLite.

Given this BigIntegerType declaration at https://github.com/ODM2/ODM2PythonAPI/blob/development/odm2api/ODM2/models.py#L10:

BigIntegerType = BigInteger()
BigIntegerType = BigIntegerType.with_variant(sqlite.INTEGER(), 'sqlite')
BigIntegerType = BigIntegerType.with_variant(postgresql.BIGINT(), 'postgresql')
BigIntegerType = BigIntegerType.with_variant(mysql.BIGINT(), 'mysql')

I wonder if the with_variant assignment is truly needed for postgresql and mysql ... If SQLAlchemy is smart enough, it seems like would map its BigInteger() class to postgresql's and mysql's BIGINT. Maybe that's something we could test in the future, to see if those last two lines can be removed. ie, the with_variant assignments might be needed only for SQLite?

In your Results example, note that the ResultDateTime and ValidDateTime are not assigned to variant type. Only the *DateTimeUTCOffset fields are, and that's b/c they're defined as BigInt (see below).

Other interesting/annoying odm2api observations:

• BigInt is used mainly for primary key fields (and obviously foreign key fields); eg, ResultID. However, this is inconsistent. Some primary keys that would logically should probably be BigInt too, are not -- eg, SamplingFeatureID, FeatureActionID.
• DateTimeUTCOffset fields. It's absolutely crazy that Results.ResultDateTimeUTCOffset and Results.ValidDateTimeUTCOffset are defined as BigInt! Totally unnecessary. Plus it's inconsistent with TimeSeriesResultValues.ValueDateTimeUTCOffset, which is defined as a plain Int

Anyway, we should follow up with an issue or two on the odm2api about these type assignments, for future reference (not for immediate implementation).

[horsburgh]

@emiliom - some of these inconsistencies may be introduced in the python script that produces the blank schema scripts for ODM2. We should look at the DBWrench file to see if the inconsistencies are there or whether they get introduced when the conversion is made for each RDBMS.

[emiliom]

@emiliom - some of these inconsistencies may be introduced in the python script that produces the blank schema scripts for ODM2. We should look at the DBWrench file to see if the inconsistencies are there or whether they get introduced when the conversion is made for each RDBMS.

Thanks for chiming in, @horsburgh! That's a good point. My comments about ODM2 type declarations were based on the DbWrench ODM2 data model diagrams. I didn't check the blank schema DDL's.

[emiliom]

@lsetiawan, to conclude:

• This PR looks good for merging. ie, no errors I can see.
• The datetime handling code change seems unnecessary for RDBMS types except SQLite, and SQLite is likely our least important backend use case for the REST API, at least initially. I'm inclined to just merge as is, but let's keep in mind that this code may introduce an inefficiency for the sake of supporting an RDBMS type that's likely the least important one for the REST API.
• Let's keep track of my comments above (via odm2api issues), for future ODM2 schema and odm2api improvements that would make most of the changes in this PR unnecessary.

I'll wait a bit for comments from you, before merging.

[lsetiawan]

This PR looks good for merging. ie, no errors I can see.

Great!

The datetime handling code change seems unnecessary for RDBMS types except SQLite, and SQLite is likely our least important backend use case for the REST API, at least initially. I'm inclined to just merge as is, but let's keep in mind that this code may introduce an inefficiency for the sake of supporting an RDBMS type that's likely the least important one for the REST API.

Hmm.. interesting. So this variant type for datetime is supposed to only show up when I'm using sqlite? I think in my case, it's showing up as variant because i'm not actually getting the types from the object I'm querying. Seems like these DateTimeType only shows up within resultvalues, so maybe the handling should go in resultvalues serializers.

[lsetiawan]

Merging so I can continue development.

[emiliom]

Sorry I was forgetting to merge. Glad you did.

Hmm.. interesting. So this variant type for datetime is supposed to only show up when I'm using sqlite?

Yes, according to this line

I think in my case, it's showing up as variant because i'm not actually getting the types from the object I'm querying.

Not sure I follow what you're saying.

Seems like these DateTimeType only shows up within resultvalues, so maybe the handling should go in resultvalues serializers.

Well, they're showing up in the results example you listed above, and that's consistent with what I found (see my long comment, above).

Anyway, we'll get back to this some other time. The PR is merged.