Remove most of parse create table #1443
Conversation
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| table := clickhouse.Table{ |
There was a problem hiding this comment.
I still like the idea of having this behind constructor, esp. given that this repeats in few places.
Of course I'd make clickhouse.NewTable() take table name and list of columns not the CREATE TABLE ... string 😉
There was a problem hiding this comment.
Totally agree, but likely in next PR.
| ClusterName string `default:""` | ||
| Cols map[string]*Column | ||
| Config *ChTableConfig | ||
| Indexes []IndexStatement |
mieciu
left a comment
mieciu
left a comment
There was a problem hiding this comment.
A very much needed refactor, great job!
I'd still prefer to have clickhouse.Table object not being initialized directly, but not going to push back because of that.
I will, though I’ll probably merge it as-is. I’ve already significantly expanded the scope of my work just to fix a tiny bug. But instead of adding more duct tape (fix table names with dots), I chose a major simplification approach (don't rely on parsing of create tables). |
|
/run-it |
|
/run-it |
1 similar comment
|
/run-it |
|
/run-it |
1 similar comment
|
/run-it |
This is stage 2 of removing the parse create table. Not completely, and still no logic to just rely on schema, but it updates all tests as well as vastly reduces the scope of the create table parser.
As a side-effect, it bugfixes tables with dots in the names.