Add generic field for engine configuration#270
Conversation
Surfaces ASTSizeLimit.warning_limit and ASTSizeLimit.exception_limit as :ast_size.warning_limit and :ast_size.exception_limit keys in the configure dict. Both are omitted when zero (the default). Guards all access to ast_size_limit with has_proto_field since the field is only set when at least one limit is non-zero. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
701abb1 to
047616a
Compare
|
As per this discussion, i'll redo this PR to add a generic key-value part of |
Drops the ASTSizeLimit message in favour of map<string, Value>
configuration_values on Configure (field 3). Known keys (semantics_version,
ivm.maintenance_level) are still extracted into structured fields; all other
config-dict entries flow into configuration_values.
Also extends the meta type system and Go codegen to support map<string, Value>
proto fields: type_env.py now resolves message-typed map values, and two new
builtins (value_map_from_pairs / value_map_to_pairs) generate typed Go helpers
(valueMapFromPairs / valueMapToPairs) that return/accept map[string]*pb.Value
instead of the generic map[string]interface{} / map[string]string variants.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove ASTSizeLimit equality (message no longer exists) and replace ast_size_limit with configuration_values in Configure ==, hash, isequal. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
davidwzhao
changed the title
| // A generic configuration dictionary. The engine can choose how to interpret any entries | ||
| // in this config dict. | ||
| map<string, Value> configuration_values = 3; |
There was a problem hiding this comment.
I have thought about this a bit more now and I sadly think the whole Configure approach was a misstep.
Anyways, I think the TL;DR is that what you're introducing here is fine, with one small clarification:
| // A generic configuration dictionary. The engine can choose how to interpret any entries | |
| // in this config dict. | |
| map<string, Value> configuration_values = 3; | |
| // A generic configuration dictionary. The engine can choose how to interpret any entries | |
| // in this config dict. Absence of an entry must be interpreted following protobuf default | |
| // value semantics. | |
| map<string, Value> configuration_values = 3; |
This lets us relax the following two requirements from the old design doc:
The client must specify all parameters explicitly in the protobuf (otherwise it would be up to the engine version to determine the behaviour).
When (pretty-)printing LQP, all; configuration values must be printed, even if they match the current default configuration. This ensures that an old snapshot test or old log message will still be interpreted correctly.
This was overeager and is only needed if the engine is allowed to flip the default behaviour without also bumping the LQP semantic version. By disallowing that, clients can safely omit anything where the protobuf default value is what they want.
There was a problem hiding this comment.
Hmm, why do absent entries need to follow protobuf default value semantics? The engine should be free to choose whatever default it wants, as long as it stays consistent within the same LQP version, right?
There was a problem hiding this comment.
I guess you're right. It doesn't have to be protobuf specifically, it's more like absence should default to the safe behaviour and defaults shouldn't change between LQP versions, yes 👍 So feature flags should be implemented as enable_xyz not disable_xyz.
2 participants
This PR adds a field
configuration_valuesto theConfiguremessage. The original motivation was to support client configuration of the engine-side AST size limit in the optimizer. But it makes more sense to just add a generic configuration field, where the engine can decide what config options to support without needing LQP changes.