From 13ef7d6d6a178b06dffd592c4b49f46b9cda4410 Mon Sep 17 00:00:00 2001 From: Nicholas Junge Date: Tue, 18 Jul 2023 21:00:10 +0200 Subject: [PATCH] Carry over hash value for all literal types in remote caching (#378) Previously, the hash set on an input literal would be ignored if the literal was of either the `Collection` or `Map` type. This is fixed here by moving the hash-checking branch up in front of the special type logic, so that an empty literal with hash is constructed in either case if a hash is found. Adds a regression test to verify the hashes are carried over into the representer literals for both map and collection types. Signed-off-by: Nicholas Junge --- go/tasks/pluginmachinery/catalog/hashing.go | 15 ++--- .../pluginmachinery/catalog/hashing_test.go | 58 +++++++++++++++++++ 2 files changed, 66 insertions(+), 7 deletions(-) diff --git a/go/tasks/pluginmachinery/catalog/hashing.go b/go/tasks/pluginmachinery/catalog/hashing.go index 79ba05a2b2..95d6b0e419 100644 --- a/go/tasks/pluginmachinery/catalog/hashing.go +++ b/go/tasks/pluginmachinery/catalog/hashing.go @@ -13,10 +13,17 @@ var emptyLiteralMap = core.LiteralMap{Literals: map[string]*core.Literal{}} // Hashify a literal, in other words, produce a new literal where the corresponding value is removed in case // the literal hash is set. func hashify(literal *core.Literal) *core.Literal { + // If the hash is set, return an empty literal with the same hash, + // regardless of type (scalar/collection/map). + if literal.GetHash() != "" { + return &core.Literal{ + Hash: literal.GetHash(), + } + } + // Two recursive cases: // 1. A collection of literals or // 2. A map of literals - if literal.GetCollection() != nil { literals := literal.GetCollection().Literals literalsHash := make([]*core.Literal, 0) @@ -45,12 +52,6 @@ func hashify(literal *core.Literal) *core.Literal { } } - // And a base case that consists of a scalar, where the hash might be set - if literal.GetHash() != "" { - return &core.Literal{ - Hash: literal.GetHash(), - } - } return literal } diff --git a/go/tasks/pluginmachinery/catalog/hashing_test.go b/go/tasks/pluginmachinery/catalog/hashing_test.go index fae0606d0b..c9b2a78595 100644 --- a/go/tasks/pluginmachinery/catalog/hashing_test.go +++ b/go/tasks/pluginmachinery/catalog/hashing_test.go @@ -550,6 +550,64 @@ func TestHashLiteralMap_LiteralsWithHashSet(t *testing.T) { }, }, }, + { + name: "literal map containing hash", + literal: &core.Literal{ + Value: &core.Literal_Map{ + Map: &core.LiteralMap{ + Literals: map[string]*core.Literal{ + "hello": { + Value: &core.Literal_Scalar{ + Scalar: &core.Scalar{ + Value: &core.Scalar_Primitive{ + Primitive: &core.Primitive{ + Value: &core.Primitive_StringValue{ + StringValue: "world", + }, + }, + }, + }, + }, + }, + }, + }, + }, + Hash: "0xffff", + }, + expectedLiteral: &core.Literal{ + Value: nil, + Hash: "0xffff", + }, + }, + { + name: "literal collection containing hash", + literal: &core.Literal{ + Value: &core.Literal_Collection{ + Collection: &core.LiteralCollection{ + Literals: []*core.Literal{ + { + Value: &core.Literal_Scalar{ + Scalar: &core.Scalar{ + Value: &core.Scalar_Primitive{ + Primitive: &core.Primitive{ + Value: &core.Primitive_Integer{ + Integer: 42, + }, + }, + }, + }, + }, + }, + }, + }, + }, + Hash: "0xabcdef", + }, + expectedLiteral: &core.Literal{ + Value: nil, + Hash: "0xabcdef", + }, + }, } for _, tt := range tests {