feat: Implement asynchronous getSchemas method and prepare for not querying schemas and server information on initialization #124
Conversation
…ge where the schemas aren't loaded on init
jimmycallin
changed the title
jimmycallin
changed the title
There was a problem hiding this comment.
It looks like we might to run into problems with some usage. The schemas are currently being used to decode the server response (for the de-duplication functionality).
call -> decode -> _mergeEntity -> getIdentifyingKey -> getPrimaryKeyAttributes -> this.schemas
There is also a subtle change in default values that might be better to hold off on until we do a new major.
source/session.ts
Outdated
| @@ -83,7 +87,7 @@ export class Session { | |||
| apiKey: string, | |||
| { | |||
| autoConnectEventHub = false, | |||
| serverInformationValues, | |||
| serverInformationValues = [], | |||
There was a problem hiding this comment.
This is potentially a breaking change.
Previously, this would mean - use the defaults from the server.
if values is None:
values = [
'schema_hash',
'storage_scenario',
'is_timezone_support_enabled'
]
Maybe leave this for a major?
| @@ -688,6 +752,7 @@ export class Session { | |||
| * @return {Object|null} Schema definition | |||
| */ | |||
| getSchema(schemaId: string): Schema | null { | |||
There was a problem hiding this comment.
Would it make sense to move the transformation from array to map from data utils here so that it can be re-used by e.g. this method?
There was a problem hiding this comment.
If we do, would it be as a private attribute? I don't think I want to expand the scope of the api client and make it diverge from the python api more than necessary?
There was a problem hiding this comment.
The map itself, sure. Could you still replace all it's usage in data-utils by this getSchema method?
There was a problem hiding this comment.
i have now implemented the map. since it's not public, we have to still do the same mapping in the frontend codebase, so no change is necessary. please review.
As long as we are doing the deduplication, we will have to load schemas for each query, unfortunately, yes. This change does not affect that functionality, since we are still loading schema on initialization, but it would be nice to continue moving towards fixing that server side so we can remove this in a coming major version as well. |
Changes
Longterm, to reduce initial session initialization time, we want to move away from having to call the
query_schemasandquery_server_informationon initialization, and move it to being called lazily whenever asynchronous methodssession.getSchemasorsession.getServerInformationare called.This PR sets up these methods, and make some additional changes to make the future breaking change as easy as possible.
Test
Make sure the methods work as intended, and haven't introduced any breaking changes.