WIP: Add JSON serializer for ASTs and store them upon node creation

[shangyian]

Summary

This PR adds a custom JSON encoder for query ASTs: ASTEncoder. This encoder uses our own circular check so that we can short-circuit the processing of circular dependencies but not raise an error. We may want to determine what's causing these circular dependencies (it looks related to FunctionTableExpression), but that's a separate issue.

This also adds a query_ast column to NodeRevision so that every time we create a node, we can store the parsed query AST alongside it. The logic for actually using this cached AST can be done separately

Test Plan

• PR has an associated issue: AST Serialization for improving Build performance #688
• make check passes
• make test shows 100% unit test coverage

Deployment Plan

[samredai]

This makes sense to me, thanks @shangyian! So much good stuff in such few lines. And I'm understanding it right that this PR creates and stores the query ast (via the node validation) but doesn't actually utilize it yet in the SQL generation? It makes sense to break that out into a separate PR.

[samredai]

[shangyian]

Yep, this just handles serializing and storing the ast. As I mentioned below, I might try some basic deserialization to make sure it works for query building, but I'll put the actual implementation in a separate PR :)

[CircArgs]

@shangyian a few questions taking a quick peek. These are all pretty much the same question from different directions I think since they are all some information I think are used after compilation but may be ignored during this serialization

• how are parent and parent_key handled when deserializing
• does this account for potential circular references like Column <-> Table
• for Table in particular, some of the ignored attributes are only set during compilation and I think are potentially used in some build stuff, are these somehow backfilled during deserialization?

[CircArgs]

I like this pattern 🙂

[agorajek]

@shangyian this is awesome. From what you said it sounds like there still may need to be adjustments done to this code once we start deserializing and using this code?

[CircArgs]

@shangyian a few questions taking a quick peek. These are all pretty much the same question from different directions I think since they are all some information I think are used after compilation but may be ignored during this serialization

• how are parent and parent_key handled when deserializing
• does this account for potential circular references like Column <-> Table
• for Table in particular, some of the ignored attributes are only set during compilation and I think are potentially used in some build stuff, are these somehow backfilled during deserialization?

Reading on the bigger screen now...

I see this is just meant to be serialization. When I was imagining this, if I had to handle potential circular stuff like in my question and your writeup, I figured maybe a flat structure like {hash(node): node_data} could work.

[shangyian]

@CircArgs -

does this account for potential circular references like Column <-> Table

So right now it's handling the circular stuff by storing a _processed set and then just stopping the continued serialization when it comes across an AST entity that's already in _processed. This might be an issue if it turns out that we do need at least one layer of serialized circular entities.

for Table in particular, some of the ignored attributes are only set during compilation and I think are potentially used in some build stuff, are these somehow backfilled during deserialization?

Yeah, so it sounds like I might need to take a stab at deserialization and make sure that all works with this setup. If not, a flat structure like you described will probably help! I think the case where having Table fully populated with columns will be used is when we're trying to build a query that needs one or more columns from that table to be grouped or filtered on as dimensions.

[shangyian]

From what you said it sounds like there still may need to be adjustments done to this code once we start deserializing and using this code?

@agorajek It's quite possible, so I'll try setting up some basic deserialization before merging just to make sure that this setup is actually enough.