GreptimeTeam / greptimedb

An open-source, cloud-native, unified time series database for metrics, logs and events with SQL/PromQL supported. Available on GreptimeCloud.
https://greptime.com/
Apache License 2.0
4.34k stars 315 forks source link

remove or reconsider the `NumbersTable` #1827

Open waynexia opened 1 year ago

waynexia commented 1 year ago

What type of enhancement is this?

Tech debt reduction

What does the enhancement do?

we have a built-in system table NumbersTable for testing. But it's very bad implemented and brings tons of special logic.

Implementation challenges

No response

tisonkun commented 1 year ago

very bad implemented and brings tons of special logic

What kind of real issues you specify here? From my point of view, except:

} else if schema == DEFAULT_SCHEMA_NAME && table_name == NUMBERS_TABLE_NAME {
            Some(NumbersTable::table(NUMBERS_TABLE_ID))

This table impl is barely an internal testing table for convenient testing.

If we don't want it pollute the main logic but only whitebox table tests, we can exclude its usage in the main process (planning/name resolution) and annotate it with #[cfg(test)]. What do you think?

cc @waynexia

waynexia commented 1 year ago

From my point of view, except:

That's one.

It also breaks a system-level invariant that every table should contain one TIME INDEX column.

If we don't want it pollute the main logic but only whitebox table tests, we can exclude its usage in the main process (planning/name resolution) and annotate it with #[cfg(test)]. What do you think?

Blackbox tests use this table more often. So #[cfg(test)] might not work. But for most cases in blackbox and whitebox tests we can use other ways to workaround NumbersTable. Like creating a temporary real table.

tisonkun commented 1 year ago

It also breaks a system-level invariant that every table should contain one TIME INDEX column.

@waynexia How do we implement it or how can we check this invariant?

I found MemTable::default_numbers_table that may replace the NumbersTable but it doesn't seem to have TIME INDEX also.

Besides, it is expected to remove:

} else if schema == DEFAULT_SCHEMA_NAME && table_name == NUMBERS_TABLE_NAME {
            Some(NumbersTable::table(NUMBERS_TABLE_ID))

? I'm unsure if SELECT * FROM numbers should work or we can drop it.