QDB-15924 - Expose lazy table creation mode in batch writer to Python API - 2 - #127
QDB-15924 - Expose lazy table creation mode in batch writer to Python API - 2#127vikonix wants to merge 39 commits into
Conversation
| # Providing an explicit schema allows the writer to create the table lazily | ||
| # if it is missing. | ||
| stocks_schema = quasardb.TableSchema( | ||
| columns=[ | ||
| quasardb.ColumnInfo(quasardb.ColumnType.Double, "open"), | ||
| quasardb.ColumnInfo(quasardb.ColumnType.Double, "close"), | ||
| quasardb.ColumnInfo(quasardb.ColumnType.Int64, "volume"), | ||
| ], | ||
| ) | ||
| qdbpd.write_dataframe( | ||
| df, | ||
| c, | ||
| "stocks", | ||
| create_schemas={"stocks": stocks_schema}, | ||
| ) |
There was a problem hiding this comment.
I think this is not a great example here, as this particular example already created the table. This should be a separate example.
| _column_schema.clear(); | ||
| _column_schema.reserve(_column_infos.size()); |
There was a problem hiding this comment.
Why do you call this column schema but the outer API table schema ? aren't they the same?
| void staged_table::prepare_table_schema(qdb_exp_batch_push_table_schema_t & table_schema) | ||
| { | ||
| table_schema.shard_size = static_cast<qdb_duration_t>(_shard_size.count()); | ||
| table_schema.ttl = (_ttl == std::chrono::milliseconds::zero()) | ||
| ? qdb_ttl_disabled | ||
| : static_cast<qdb_duration_t>(_ttl.count()); | ||
|
|
||
| const auto & columns = prepare_column_schema(); | ||
| table_schema.columns = columns.data(); | ||
| table_schema.column_count = columns.size(); | ||
| } |
There was a problem hiding this comment.
Why is _column_schema stored as part of the staged table object, but table schema not?
(I know the answer, but this should be documented / commented)
| , _creation_allowed(table.has_local_schema()) | ||
| , _shard_size(table.get_shard_size()) | ||
| , _ttl(table.get_ttl()) |
There was a problem hiding this comment.
Aren't shard size and ttl part of the table schema? Shouldn't we potentially refactor the way this is represented, rather than have a lot of different class parameters on staged table? I.e. keep the schema in a separate struct.
Also: why do we store shard size and ttl as part of the constructor, but not the columns? Are you sure this is the place where we should store it?
| _table_name.clear(); | ||
| _column_infos.clear(); | ||
| _columns_data.clear(); | ||
| _column_schema.clear(); |
There was a problem hiding this comment.
I'm not a fan of "column schema" as name, it seems to conflict with "column infos", it's unclear what it is.
| std::chrono::milliseconds ttl) | ||
| : entry{h, a} | ||
| , _has_indexed_columns(false) | ||
| , _has_local_schema(true) |
There was a problem hiding this comment.
I think we should represent the table schema as a std::optionaltable::schema or whatever, then we don't need the has local schema anymore.
| , _columns{normalize_local_schema(std::move(columns))} | ||
| , _ttl{ttl} | ||
| , _shard_size{shard_size} |
There was a problem hiding this comment.
Should be stored as a table::schema
| @dataclass | ||
| class TableSchema: | ||
| """Schema used to create a missing time-series table during a writer push.""" | ||
|
|
||
| columns: List[ColumnInfo] | ||
| shard_size: timedelta = timedelta(days=1) | ||
| ttl: timedelta = timedelta(0) |
There was a problem hiding this comment.
What's the purpose of this? Isn't the standard in the API that we define these classes in C++, if they're used from C++?
If this is not used from C++, perhaps we should?
|
|
||
| if (creation_mode == qdb_exp_batch_create_tables) | ||
| { | ||
| if (staged_table.creation_allowed() == false) |
There was a problem hiding this comment.
I would add [[unlikely]] here
| auto & batch_table = batch.at(cur); | ||
| qdb_exp_batch_creation_mode_t table_creation_mode = qdb_exp_batch_dont_create; | ||
|
|
||
| if (creation_mode == qdb_exp_batch_create_tables) |
There was a problem hiding this comment.
can't "creation mode" and "creation allowed" be folded into a single argument?
i.e. if a schema is provided, creation is enabled, if not, it's not enabled.
will simplify things
2 participants
No description provided.