Skip to content

Conversation

@kyessenov
Copy link
Collaborator

@kyessenov kyessenov commented Feb 26, 2020

--
297443901:

Use absl::Status.
Add status_macros.h for the explicit status macros import.
Make `CelExpressionFlatImpl` non-copyable to avoid a potential issue with its vector of unique pointers.

--
297297141:

Temporarily disable printing of test values

296507025:

Note that interpreter tracing behaves differently when the constant folding is
enabled.
--
296273889:

Add end to end tests for unknown functions and attributes. Add benchmarks (mostly characterizing worst-case merge performance).

--
296273780:

Add support for unknown function results.
--
296269903:

Add option for unknown function results. Final implementation in next change.
--
296236495:

Add support for nested comprehensions. Before this, reusing the accumulator or iter var in a nested comprehension could put the evaluation frame in a bad state and cause a status.
--
295763189:

Update references to unknownattribute set that should refer to an unknown set.
--
295056234:

Add UnknownSet type -- this just coordinates updating the underlying attribute set and function results set.
--
295026542:

Add UnknownFunctionResult and UnknownFunctionResultSet types.
--
293733801:

absl::GetWeekday() no longer requires an absl::CivilDay argument.
Any absl::Civil* variety will do ... they all have a weekday.

--
293705273:

Adding test that verify behavior of CEL ternary operator for different cases (true/false/error/unknown)
--
292626176:

Fix ConstantFoldingTransform bug.
--
--
292565730:

Separate out AttributeTrail and UnknownsUtility into their own build targets.
--
291997941:

Adding Unknowns support to CEL C++ Evaluator
- Adding unknown_patterns to Activation
- Apply changes to evaluator core to support unknowns:
  - attributes stored in value stack along with corresponding values;
  - patterns stored in execution frame
  - Unknowns support in ExecutionStep implementations.
--
291392455:

CEL C++ interpreter: add the missing int->string conversion function.
--
287599493:

Add option for opting out of treating warnings as fatal errors.
--
286093278:

Expose evaluation state from Expression parsing.

CelExpression can now initialize and use an opaque "CelEvaluationState".  This type holds all the mutable state used by CelExpression during the Trace and Evaluate calls.  Instead of allocating and initializing this state each time Evalue or Trace is called, the user can elect to Initialize a CelEvaluationState prior to those calls and pass it into the newly provided overrides.  This allows the user to remove these allocations when the CelExpression is called frequently under certain conditions (performance, realtime).
--
286061190:

Remove active allocation paths from ExecutionFrame IterVars.

--
286039529:

Remove active allocation paths from ValueStack.

--
285120619:

Remove unused absl::string_view variables.

--
285016935:

Add method for getting build warnings associated with a built expression.
--
284968502:

Fix the naming  for AddResolvableEnum/RemoveResolvableEnum methods in CelExpressionBuilder
--
284848907:

Demote function resolution errors at evaluation time to a warning. Client must opt out of treating warning as a fatal error (maintains current behavior).
--
--
284594209:

Removed direct access to ExecutionFrame iteration variable map.

PiperOrigin-RevId: 297443901

--
297443901:

BEGIN_PUBLIC
Use absl::Status.
Add status_macros.h for the explicit status macros import.
Make `CelExpressionFlatImpl` non-copyable to avoid a potential issue with its vector of unique pointers.
END_PUBLIC

Replace `util::Status` with `absl::Status` and canonical codes throughout.
Add `static_cast<uint8>(char)` to get rid of the compiler warning about signed chars.
Fully qualify Expr and SourceInfo in a few places to facilitate v1alpha1 rewrite.
Change tests using `CelExpressionFlatImpl` since clang-8 is unable to synthesize the move constructor for its vector of unique pointers.
Refactor code using `StatusIs` since OSS testing has no status macros.
Refactor RegisterBuiltinFunctions into a few functions to reduce its length.
--
297297141:

Disable printing of test values

This is done in preparation for cl/296247273 which changes the names of the
color constants. The intention is to reinstate the additional information
display after that has landed.

BEGIN_PUBLIC
Temporarily disable printing of test values
END_PUBLIC
--
297266899:

LSC: Add std:: qualifications to all references to std::string and std::basic_string.

Adding these qualifications will make google3 C++ more portable, allow the global using std::string declarations in the google3 copy of the C++ standard library to be deleted, and bring google3 C++ in line with how the rest of the world uses C++. This completes the work started in go/lsc-add-std and go/std-type-qualification.

LSC documentation: go/lsc-std-string

Tested:
    tap_presubmit: http://test/OCL:296203366:BASE:296180783:1582224434313:c3cf9bf1
    Some tests failed; test failures are believed to be unrelated to this CL
--
296969963:

Mock CEL object
--
296958295:

LSC: Replace gtl swiss table forwarding headers with their //third_party/absl/container equivalents.

Background: gtl/util/(node|flat)_hash_(map|set).h are deprecated forwarding headers. This CL cleans up references to those header files.

LSC document: go/absl-cleanup-lsc

Tested:
    TAP --sample ran all affected tests and none failed
    http://test/OCL:296524469:BASE:296537645:1582335970682:be92947f
--
296507025:

BEGIN_PUBLIC
Note that interpreter tracing behaves differently when the constant folding is
enabled.
END_PUBLIC
--
296273889:

BEGIN_PUBLIC
Add end to end tests for unknown functions and attributes. Add benchmarks (mostly characterizing worst-case merge performance).
END_PUBLIC
--
296273780:

BEGIN_PUBLIC
Add support for unknown function results.
END_PUBLIC
--
296269903:

BEGIN_PUBLIC
Add option for unknown function results. Final implementation in next change.
END_PUBLIC
--
296236495:

BEGIN_PUBLIC
Add support for nested comprehensions. Before this, reusing the accumulator or iter var in a nested comprehension could put the evaluation frame in a bad state and cause a status.
END_PUBLIC
--
295763189:

BEGIN_PUBLIC
Update references to unknownattribute set that should refer to an unknown set.
END_PUBLIC
--
295056234:

BEGIN_PUBLIC
Add UnknownSet type -- this just coordinates updating the underlying attribute set and function results set.
END_PUBLIC
--
295026542:

BEGIN_PUBLIC
Add UnknownFunctionResult and UnknownFunctionResultSet types.
END_PUBLIC
--
294785557:

Project import generated by Copybara.
--
293733801:

absl::GetWeekday() no longer requires an absl::CivilDay argument.
Any absl::Civil* variety will do ... they all have a weekday.

Tested:
    tap_presubmit: http://test/OCL:293299235:BASE:293282957:1580966223484:4091c83f
    Some tests failed; test failures are believed to be unrelated to this CL
--
293705273:

BEGIN_PUBLIC
Adding test that verify behavior of CEL ternary operator for different cases (true/false/error/unknown)
END_PUBLIC
--
292626176:

BEGIN_PUBLIC
Fix ConstantFoldingTransform bug.
END_PUBLIC

bugfix: add struct_expr.message_name in ConstantFoldingTransform
--
292579803:

Updating Guitar workflow BUILD target dependencies to reflect recent runs.

go/prax is a tool that discovers missing dependencies in Guitar workflow BUILD targets by analyzing recent workflow runs.

This CL was automatically created by go/prax to fix some of your BUILD files.

go/prax found an update to this BUILD file to more closely match dependencies.

NOTICE:
This CL adds dependencies to one or more Guitar workflow BUILD targets. Please be aware that while workflows without dependencies always run on presubmits, once dependencies are added, Guitar will only run them when affected by the pending CL. Missing dependencies may lead to workflows not running when they should.

IMPORTANT:
Not all dependencies can be extracted from a Guitar invocation. Here is a list of targets that are not available in previous Guitar invocations:
* The SandMan GCL definition (because not a target). This can be added manually using a borgcfg_library target with all the SandMan GCL files.
* All the MPMs that are just fetched (not built) by the Borg jobs started by SandMan. The genmpm targets can be added manually.

Make sure to manually add all the missing dependencies to your workflow or Guitar will skip the test at presubmit, even though the CL may cause the test to fail.

For more details: go/guitar-3-deps

This is based on your BUILD file @ cl/292377696
If you do not think this is correct, please file a bug @ go/prax-file-a-bug

This CL looks good? Just LGTM and Approve it!

What else can you do?
* Suggest a fix on the CL (go/how-to-suggest-fix).
* Revert this CL, by replying "REVERT: <provide reason>"
* Reassign it to a more suitable reviewer with sufficient approval rights.
* Set enable_auto_deps=False in your BUILD target (go/gbe#common-attributes-behavior) to stop receiving PRAX updates for this BUILD target.
--
292565730:

BEGIN_PUBLIC
Separate out AttributeTrail and UnknownsUtility into their own build targets.
END_PUBLIC

clang-migrate spec:
old_header: "third_party/cel/cpp/eval/eval/evaluator_core.h"
new_header: "third_party/cel/cpp/eval/eval/unknowns_utility.h"
old_ns: "google::api::expr::runtime"
new_ns: "google::api::expr::runtime"
renames {
  kind: CLASS
  old_symbol: "google::api::expr::runtime::UnknownsUtility"
  new_symbol: "google::api::expr::runtime::UnknownsUtiltiy"
}
old_depend_on_new: true

clang-migrate spec:
old_header: "third_party/cel/cpp/eval/eval/evaluator_core.h"
new_header: "third_party/cel/cpp/eval/eval/attribute_trail.h"
old_ns: "google::api::expr::runtime"
new_ns: "google::api::expr::runtime"
renames {
  kind: CLASS
  old_symbol: "google::api::expr::runtime::AttributeTrail"
  new_symbol: "google::api::expr::runtime::AttributeTrail"
}
old_depend_on_new: true
--
292387727:

Parse CEL AST expression and extract destination IP prefix set

BEGIN_PUBLIC
Internal change
END_PUBLIC

- Arcus sends CEL AST expression to NATM.
- This parser will extract destination IP prefix set from the AST expression.
--
291997941:

BEGIN_PUBLIC
Adding Unknowns support to CEL C++ Evaluator
- Adding unknown_patterns to Activation
- Apply changes to evaluator core to support unknowns:
  - attributes stored in value stack along with corresponding values;
  - patterns stored in execution frame
  - Unknowns support in ExecutionStep implementations.
END_PUBLIC
--
291392455:

BEGIN_PUBLIC
CEL C++ interpreter: add the missing int->string conversion function.
END_PUBLIC
--
290287046:

Update form definition compilation script to parse expression using the CEL cpp library instead of parsing & checking it via the expr service.

BEGIN_PUBLIC
Internal change
END_PUBLIC
--
287599493:

BEGIN_PUBLIC
Add option for opting out of treating warnings as fatal errors.
END_PUBLIC
--
286093278:

BEGIN_PUBLIC
Expose evaluation state from Expression parsing.

CelExpression can now initialize and use an opaque "CelEvaluationState".  This type holds all the mutable state used by CelExpression during the Trace and Evaluate calls.  Instead of allocating and initializing this state each time Evalue or Trace is called, the user can elect to Initialize a CelEvaluationState prior to those calls and pass it into the newly provided overrides.  This allows the user to remove these allocations when the CelExpression is called frequently under certain conditions (performance, realtime).
END_PUBLIC
--
286061190:

BEGIN_PUBLIC
Remove active allocation paths from ExecutionFrame IterVars.
END_PUBLIC

The current method of setting the variable's values (and clearing them) involves adding/removing from a map.  Instead, the expression builder now tracks all variable names used in the expression, and those are used to initialize the frame's map.  During evaluation, the variables are now set on the existing entry in the map, resulting in no allocations from the map during these operations.  A "cleared" value is represented by a default value CelValue, not a non-existing map entry. (allocations from CelValue::operator=() still need to be addressed).
--
286039529:

BEGIN_PUBLIC
Remove active allocation paths from ValueStack.
END_PUBLIC

The ValueStack no longer directly manipulates a vector, but treats it as a fixed size array and just adjusts values up and down as it inserts them.
--
285120619:

Remove unused absl::string_view variables.

For various weird reasons, Clang does not currently warn
about unused absl::string_view objects (because it thinks the constructor
may have side-effects).

In order to make absl::string_view interchangable with std::string_view,
this needs to be cleaned up.

This patch removes all unused string_view objects. It /does not/ attempt
to fix any underlying bugs signaled by the unused variable.

If you have a better fix, please suggest it.

Tested:
    TAP --sample ran all affected tests and none failed
    http://test/OCL:285082973:BASE:285111449:1576127415846:786161bf
--
285016935:

BEGIN_PUBLIC
Add method for getting build warnings associated with a built expression.
END_PUBLIC
--
284968502:

BEGIN_PUBLIC
Fix the naming  for AddResolvableEnum/RemoveResolvableEnum methods in CelExpressionBuilder
END_PUBLIC
--
284848907:

BEGIN_PUBLIC
Demote function resolution errors at evaluation time to a warning. Client must opt out of treating warning as a fatal error (maintains current behavior).
END_PUBLIC

The use case for this is for clients that may have slower rollouts than the reference environment. (e.g. expression and compiler expects all evalutors to have a new function but some clients lag and don't add the new function to their registries yet.)
--
284618467:

BEGIN_PUBLIC
Squash public notes on export.
END_PUBLIC
--
284594209:

BEGIN_PUBLIC
Removed direct access to ExecutionFrame iteration variable map.
END_PUBLIC

This change allows us to revamp the inner workings of the ExecutionFrame's iterator variable map without exposing those changes to client code of the class.

PiperOrigin-RevId: 297443901
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no CLA not present label Feb 26, 2020
@kyessenov kyessenov added cla: yes CLA present and removed cla: no CLA not present labels Feb 26, 2020
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@kyessenov
Copy link
Collaborator Author

CLA manually confirmed.

@kyessenov kyessenov merged commit 80e1cca into master Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes CLA present

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants