chore: add `fieldValueDictionary` accessor to `dictionaryGetBatch` #65

poppoerika · 2022-11-15T06:05:58Z

Added fieldValueDictionary accessor to dictionaryGetBatch to return an array of field/value pairs.
Also, updated dictionary accessor to fieldValueDictionary for dictionaryFetch to be consistent with naming.
Corresponding tests are also updated.

Closes #53

…ield-value pairs chore: add fieldsVal uesArray accessor to return an array containing field-value pairs

cprice404 · 2022-11-15T16:54:14Z

src/Cache/CacheOperationTypes/CacheOperationTypes.php

@@ -1020,6 +1024,11 @@ public function valuesArray(): array
        return $ret;
    }

+    public function fieldValueDictionary(): array


I think by our naming conventions this would start with value. I think maybe it would be valueDictionary but let's get @pgautier404 to weigh in.

Yes, everything should begin with value. I thought we were going to go with something like valueArray() because dictionaries are arrays in PHP, but I could go with either name as long as it's consistent.

See #65 (comment)

and #65 (comment)

cprice404 · 2022-11-15T16:55:02Z

src/Cache/CacheOperationTypes/CacheOperationTypes.php

@@ -1020,6 +1024,11 @@ public function valuesArray(): array
        return $ret;


also i think we might want to change the name of valuesArray to just responses? Since it's not actually returning values directly, but rather, an array of objects that may have values in them?

@cprice404 this actually is extracting the values from the responses and returning them.

See #65 (comment)

cprice404 · 2022-11-15T16:56:14Z

src/Cache/_ScsDataClient.php

@@ -576,7 +576,7 @@ public function dictionaryGetBatch(string $cacheName, string $dictionaryName, ar
            return new CacheDictionaryGetBatchResponseError(new UnknownError($e->getMessage()));
        }
        if ($dictionaryGetBatchResponse->hasFound()) {
-            return new CacheDictionaryGetBatchResponseSuccess($dictionaryGetBatchResponse);
+            return new CacheDictionaryGetBatchResponseSuccess($dictionaryGetBatchResponse, fields: $fields);


I think you have another PR up that may have conflicts with this one, yes? Where this class gets renamed to *DictionaryGetFields*? Just calling it out to make sure that we end up with the correct one before the final merge.

Yeah this is the PR for that: #64

poppoerika · 2022-11-15T18:19:59Z

@pgautier404 @cprice404
We want to return two values from dictionaryGetFields (which is dictionaryGetBatch in this branch still)

an array containing CacheDictionaryGetResponse's values; in case of HIT, it's a string value, and in vase of MISS, it's null (currently this is called valuesArray)
an array containing field/value pairs, which I named fieldValueDictionary.

For 1, I think we can keep the name as is. Or we can rename this to responses to align with our .NET SDK, although it doesn't follow the value<type> naming convention.
For 2, I wanted to hear your opinion. I chose fieldValueDictionary because I wanted to differentiate it from valuesArray since it contains field/value pairs compared to just values. How about fieldStringValueStringArray to represent a content inside?
This suggestion will be changed depending on our decision for 1.

pgautier404 · 2022-11-15T19:38:45Z

@pgautier404 @cprice404 We want to return two values from dictionaryGetFields (which is dictionaryGetBatch in this branch still)
1. an array containing `CacheDictionaryGetResponse`'s values; in case of HIT, it's a string value, and in vase of MISS, it's `null` (currently this is called `valuesArray`)

2. an array containing field/value pairs, which I named `fieldValueDictionary`.
For 1, I think we can keep the name as is. Or we can rename this to responses to align with our .NET SDK, although it doesn't follow the value<type> naming convention. For 2, I wanted to hear your opinion. I chose fieldValueDictionary because I wanted to differentiate it from valuesArray since it contains field/value pairs compared to just values. How about fieldStringValueStringArray to represent a content inside? This suggestion will be changed depending on our decision for 1.

The .NET SDK is returning responses without extracting values from them in a Responses property. There are also methods for returning the extracted values of responses as a list of strings or a list of byte arrays. It doesn't return key/value pairs anywhere. We need to do the same. Like Chris suggested, the function returning the list of responses should just be called responses, and the list of extracted values should be called valuesString().

@cprice404 @malandis can you sanity check me here please?

cprice404 · 2022-11-15T19:49:08Z

@pgautier404 @cprice404 We want to return two values from dictionaryGetFields (which is dictionaryGetBatch in this branch still)
1. an array containing `CacheDictionaryGetResponse`'s values; in case of HIT, it's a string value, and in vase of MISS, it's `null` (currently this is called `valuesArray`)

2. an array containing field/value pairs, which I named `fieldValueDictionary`.
For 1, I think we can keep the name as is. Or we can rename this to responses to align with our .NET SDK, although it doesn't follow the value<type> naming convention. For 2, I wanted to hear your opinion. I chose fieldValueDictionary because I wanted to differentiate it from valuesArray since it contains field/value pairs compared to just values. How about fieldStringValueStringArray to represent a content inside? This suggestion will be changed depending on our decision for 1.
The .NET SDK is returning responses without extracting values from them in a Responses property. There are also methods for returning the extracted values of responses as a list of strings or a list of byte arrays. It doesn't return key/value pairs anywhere. We need to do the same. Like Chris suggested, the function returning the list of responses should just be called responses, and the list of extracted values should be called valuesString().

@cprice404 @malandis can you sanity check me here please?

My intention for this API is that there will only be two accessors on this response type after we make the changes:

A values dictionary
An array containing the raw responses (values not extracted, so no .value prefix necessary)

In .NET we will be removing the methods/properties for "accessing the extracted values as a list of strings or as a list of byte arrays".

poppoerika · 2022-11-15T19:57:06Z

@pgautier404 @cprice404 We want to return two values from dictionaryGetFields (which is dictionaryGetBatch in this branch still)
1. an array containing `CacheDictionaryGetResponse`'s values; in case of HIT, it's a string value, and in vase of MISS, it's `null` (currently this is called `valuesArray`)

2. an array containing field/value pairs, which I named `fieldValueDictionary`.
For 1, I think we can keep the name as is. Or we can rename this to responses to align with our .NET SDK, although it doesn't follow the value<type> naming convention. For 2, I wanted to hear your opinion. I chose fieldValueDictionary because I wanted to differentiate it from valuesArray since it contains field/value pairs compared to just values. How about fieldStringValueStringArray to represent a content inside? This suggestion will be changed depending on our decision for 1.
The .NET SDK is returning responses without extracting values from them in a Responses property. There are also methods for returning the extracted values of responses as a list of strings or a list of byte arrays. It doesn't return key/value pairs anywhere. We need to do the same. Like Chris suggested, the function returning the list of responses should just be called responses, and the list of extracted values should be called valuesString().
@cprice404 @malandis can you sanity check me here please?
My intention for this API is that there will only be two accessors on this response type after we make the changes:

A values dictionary

An array containing the raw responses (values not extracted, so no .value prefix necessary)

In .NET we will be removing the methods/properties for "accessing the extracted values as a list of strings or as a list of byte arrays".

Got it.
I don't recall why I created valuesArray to only return values instead of responses...
I will update to the following:

responses which contain the raw responses
valuesArray which contains key/value pairs. Same as dictionary method from dictionaryFetch.

Should I use dictionary instead of valuesArray although dictionary is not a type in PHP?
If we decided to use valuesArray, I'd like to rename dictionary in dictionaryFetch to be consistent.

cprice404 · 2022-11-15T20:10:25Z

Should I use dictionary instead of valuesArray although dictionary is not a type in PHP?

It should start with values either way. valuesDictionary makes more sense to me but it might not for folks more experienced with PHP. @pgautier404 wdyt?

pgautier404 · 2022-11-15T22:49:23Z

Should I use dictionary instead of valuesArray although dictionary is not a type in PHP?

It should start with values either way. valuesDictionary makes more sense to me but it might not for folks more experienced with PHP. @pgautier404 wdyt?

I honestly couldn't speak for PHP devs other than myself, but I would personally prefer valuesDictionary and appreciate the hint about which of the polymorphic PHP array types I was supposed to be getting. When I type $response->asHit()->v I think the completion should help document how to handle the value type.

…sDictionary

…nd update tests

pgautier404 · 2022-11-16T20:23:07Z

src/Cache/CacheOperationTypes/CacheOperationTypes.php

-                    $this->responsesList[] = new CacheDictionaryGetFieldResponseMiss();
+                    $this->responses[] = new CacheDictionaryGetFieldResponseHit(null, $response->getCacheBody());
+                    $this->valuesDictionary[$fields[$counter]] = $response->getCacheBody();
+                    $counter++;


I'd like to ask you about this counter and about the numRequested param.

pgautier404 · 2022-11-16T23:02:36Z

src/Cache/_ScsDataClient.php

+        if ($dictionaryGetFieldsResponse->hasFound()) {
+            return new CacheDictionaryGetFieldsResponseSuccess($dictionaryGetFieldsResponse, fields: $fields);
        }
        return new CacheDictionaryGetFieldsResponseSuccess(null, count($fields));


This should be returning CacheDictionaryGetFieldsResponseHit and CacheDictionaryGetFieldsResponseMiss instead of trying to use the Success class to represent two different states.

pgautier404 · 2022-11-16T23:03:41Z

src/Cache/CacheOperationTypes/CacheOperationTypes.php

        if (is_null($responses) && !is_null($numRequested)) {
            foreach (range(0, $numRequested - 1) as $ignored) {
-                $this->responsesList[] = new CacheDictionaryGetFieldResponseMiss();
+                $this->responses[] = new CacheDictionaryGetFieldResponseMiss();


I think all of this is the result of trying to use a Success class where we need to be using Hit and Miss classes instead. I left a comment on _ScsDataClient that lines up with this one.

When I moved CacheDicitonaryGetBatch from our .NET incubating repo, I referenced here, which has Success and Error as CacheDictionaryGetBatchResponse.
Sorry if I missed, but are we going to update this response type to Hit, Miss, and Error instead?

You might double check with @cprice404 but I think this should follow the Hit/Miss/Error pattern since we're dealing with a single dictionary value and we can tell if our request was a miss in the corresponding data client code. It should also allow us to get rid of the $numRequested parameter and logic in the CacheDictionaryGetFieldsResponseSuccess constructor.

I think I agree with @pgautier404 , I think this was an oversight when we were switching the .NET SDK over.

Here:

https://github.com/momentohq/client-sdk-dotnet-incubating/blame/main/src/Momento.Sdk.Incubating/Internal/ScsDataClient.cs#L312

That code does not check for the "Missing" case that we got from the server. My gut is that it should be checking for that case, and should use Hit/Miss/Error pattern instead of success. That's what we do for DictionaryFetch, ListFech, etc. So we should fix that both in .NET and PHP. I created this ticket to capture it for .NET momentohq/client-sdk-dotnet-incubating#47 but yes let's go ahead and make this change here. @malandis if you know of any reason why DictionaryGetBatch/DictionaryGetFields shouldn't use Hit/Miss/Error pattern let us know.

I didn't review this closely enough to understand why that change would eliminate the need for the numRequested/counter thing but I also agree that it seems like we shouldn't need those, so if they still seem necessary after the switch to Hit/Miss/Error feel free to ping for 👀 .

Sorry for not catching this during the .NET work.

@pgautier404 @cprice404 @malandis
My understanding is we want to have 4 different response patters:

We found a dictionary and all items in the dictionary also found.

We found a dictionary but some of the items in the dictionary are missing.

We did not find a dictionary.

We had an error finding a dictionary.

Cases 1, 3, 4 are straightforward.
1 -> Hit
3 -> Miss
4 -> Error

But how about case 2? Would this be Hit although some of the items are Miss?
So Hit means that finding a dictionary not necessarily the items inside?

yes my opinion is that if the Dictionary itself is found, then case 2 is a HIT.

users may then check for individual fields by iterating over the responses and seeing which are HIT vs MISS, or they can just use valueDictionary and check to see if each key exists.

…s and corresponding tests

…wright key/value pair

pgautier404 · 2022-11-17T21:30:38Z

src/Cache/CacheOperationTypes/CacheOperationTypes.php

+        $counter = 0;
+        parent::__construct();


Kind of a nit, but the call to the parent constructor should pretty much always be the first line in a function:

Suggested change

$counter = 0;

parent::__construct();

parent::__construct();

$counter = 0;

pgautier404 · 2022-11-17T21:31:20Z

src/Cache/CacheOperationTypes/CacheOperationTypes.php

+            if ($response->getResult() == ECacheResult::Hit) {
+                $this->responses[] = new CacheDictionaryGetFieldResponseHit(null, $response->getCacheBody());
+                $this->valuesDictionary[$fields[$counter]] = $response->getCacheBody();
+            } else if ($response->getResult() == ECacheResult::Miss) {


My IDE is telling me that PHP standards prefer elseif.

Suggested change

} else if ($response->getResult() == ECacheResult::Miss) {

} elseif ($response->getResult() == ECacheResult::Miss) {

pgautier404 · 2022-11-17T21:35:17Z

I left a couple of notes on really minor changes but am good for this to merge once you have a chance to look at those!

pgautier404

LGTM!

cprice404

🚢

poppoerika added 6 commits November 14, 2022 21:51

chore: add fieldsValuesArray accessor to return an array containing f…

f7c1c1b

…ield-value pairs chore: add fieldsVal uesArray accessor to return an array containing field-value pairs

chore: add a test for fieldsValuesArray

f496ad7

chore: update dictionary accessor name to fieldValueDictionary

115c5d7

chore: update fieldValueDictionary test

a081363

chore: update dicitonaryFetch accessor name to fieldValueDictionary

db9b1ef

chore: update dictionaryFetch test to use fieldValueDictionary

e862a87

poppoerika requested review from pgautier404 and cprice404 November 15, 2022 06:06

cprice404 reviewed Nov 15, 2022

View reviewed changes

poppoerika added 6 commits November 15, 2022 15:13

fix: merge conflict

ca46bbc

chore: update variable names to match dictionary/set operations

f78391b

chore: add two properties to dictionaryGetFields: responses and value…

130ee0d

…sDictionary

chore: update dictionaryGetFields tests

e85e4f9

chore: update property name to valuesDictionary for dictionaryFetch a…

2f2db4a

…nd update tests

fix: remove extra $

43f49da

poppoerika requested a review from cprice404 November 16, 2022 05:46

pgautier404 reviewed Nov 16, 2022

View reviewed changes

poppoerika added 3 commits November 17, 2022 11:54

chore: update response types to hit/miss/error for dicitonaryGetField…

9ff328f

…s and corresponding tests

fix: move out from if and always increment to properly reference the …

5b41d08

…wright key/value pair

Merge branch 'main' into chore/dictionary-get-batch-return-array

1da7f4c

poppoerika requested a review from pgautier404 November 17, 2022 20:19

pgautier404 reviewed Nov 17, 2022

View reviewed changes

chore: declare parent as the first thing; use elseif

769be70

poppoerika requested a review from pgautier404 November 17, 2022 22:04

pgautier404 approved these changes Nov 17, 2022

View reviewed changes

cprice404 approved these changes Nov 17, 2022

View reviewed changes

poppoerika merged commit 1a1e86c into main Nov 17, 2022

malandis deleted the chore/dictionary-get-batch-return-array branch November 1, 2024 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add `fieldValueDictionary` accessor to `dictionaryGetBatch` #65

chore: add `fieldValueDictionary` accessor to `dictionaryGetBatch` #65

poppoerika commented Nov 15, 2022

cprice404 Nov 15, 2022

pgautier404 Nov 15, 2022

pgautier404 Nov 15, 2022

cprice404 Nov 15, 2022 •

edited

Loading

cprice404 Nov 15, 2022

pgautier404 Nov 15, 2022

pgautier404 Nov 15, 2022

cprice404 Nov 15, 2022

poppoerika Nov 15, 2022

poppoerika commented Nov 15, 2022

pgautier404 commented Nov 15, 2022

cprice404 commented Nov 15, 2022

poppoerika commented Nov 15, 2022

cprice404 commented Nov 15, 2022

pgautier404 commented Nov 15, 2022

pgautier404 Nov 16, 2022

pgautier404 Nov 16, 2022 •

edited

Loading

pgautier404 Nov 16, 2022

poppoerika Nov 16, 2022

pgautier404 Nov 17, 2022

cprice404 Nov 17, 2022

poppoerika Nov 17, 2022

cprice404 Nov 17, 2022

pgautier404 Nov 17, 2022

pgautier404 Nov 17, 2022

pgautier404 commented Nov 17, 2022

pgautier404 left a comment

cprice404 left a comment

		@@ -1020,6 +1024,11 @@ public function valuesArray(): array
		return $ret;

	} else if ($response->getResult() == ECacheResult::Miss) {
	} elseif ($response->getResult() == ECacheResult::Miss) {

chore: add fieldValueDictionary accessor to dictionaryGetBatch #65

chore: add fieldValueDictionary accessor to dictionaryGetBatch #65

Conversation

poppoerika commented Nov 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cprice404 Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poppoerika commented Nov 15, 2022

pgautier404 commented Nov 15, 2022

cprice404 commented Nov 15, 2022

poppoerika commented Nov 15, 2022

cprice404 commented Nov 15, 2022

pgautier404 commented Nov 15, 2022

Choose a reason for hiding this comment

pgautier404 Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgautier404 commented Nov 17, 2022

pgautier404 left a comment

Choose a reason for hiding this comment

cprice404 left a comment

Choose a reason for hiding this comment

chore: add `fieldValueDictionary` accessor to `dictionaryGetBatch` #65

chore: add `fieldValueDictionary` accessor to `dictionaryGetBatch` #65

cprice404 Nov 15, 2022 •

edited

Loading

pgautier404 Nov 16, 2022 •

edited

Loading