nodejs-datastore icon indicating copy to clipboard operation
nodejs-datastore copied to clipboard

FR: Querying a subset of an embedded entity properties.

Open vicb opened this issue 3 years ago • 5 comments

Let's say you save this entity:

  await datastore.save([{
    key: datastore.key(['TEST']);
    data: {
      emb: {
        name: 'first',
        value: 1,
      },
    },
  }]);

You can they run a query to retrieve that entity:

const [entities] = await datastore.createQuery('TEST').run();


// returns 
[{
    emb: {
      name: 'first',
      value: 1,
    },
},]

That is expected.

Now if you select only some properties, the output format would be different:

const [entities] = await datastore.createQuery('TEST').select('emb.name').run();

// returns 
[{
    "emb.name": 'first',
},]

I have not found any documentation about querying embedded entities so I am not sure if this is expected ?

Do you think it would be worth expanding the response to a partial object in the second case, ie

[{
    emb: {name": 'first'},
},]

This would make the client code consistent whether or not the query selects only a subset of the embedded property.

Note: this would have to be done on the client side.

When there is no select clause, the server returns:

Value Proto {
  meaning: ...,
  excludeFromIndexes: ...,
  entityValue: {
    properties: { name: [Object], value: [Object] },
    key: null
  },
  valueType: 'entityValue',
  propertyName: 'emb'
}

but when there is select, the server would return

Value Proto {
  meaning: 18,
  excludeFromIndexes: false,
  stringValue: 'first',
  valueType: 'stringValue',
  propertyName: 'emb.name'
}

vicb avatar Nov 21 '20 01:11 vicb

Do you think it would be worth expanding the response to a partial object in the second case, ie

[{
    emb: {name": 'first'},
},]

This would make the client code consistent whether or not the query selects only a subset of the embedded property.

Sorry, I don't quite follow. It seems you are wondering if we should make a, likely breaking, change, to the way results are returned?

crwilcox avatar Dec 01 '20 18:12 crwilcox

I think the current API is inconsistent:

const [entities] = await datastore.createQuery('TEST').run();

// returns
[{
    emb: {
       name": 'first',
       value: 1,
    },
},]

vs 

const [entities] = await datastore.createQuery('TEST').select('emb.name').run();

// returns 
[{
    "emb.name": 'first',
},]

That is the shape of the result depends on whether you have or not a select clause.

I can imagine multiple solutions for it not to be a breaking change.

  1. Add one option to return partial objects instead of path to the properties when there is a select clause.

  2. Returns both form, i.e.

const [entities] = await datastore.createQuery('TEST').select('emb.name').run();

// *would* return
[{
    "emb.name": 'first',
    emb: {name: "first"},
},]

vicb avatar Dec 01 '20 19:12 vicb

Okay, I think I understand. Going to restate to be sure though :)

Select, when selecting the mentioned field, returns the key as the select statement. In this case "emb.name".

You would prefer if the returned object was still nested as it is in the full object, even though it is a single field. And you are proposing that we could return it in both forms in the dict to allow users to use the existing way, as well as an additional way that would match the structure of the object without a select query.

crwilcox avatar Dec 01 '20 19:12 crwilcox

Yep that is what I meant.

even though it is a single field

You can select multiple fields too in which case there will be {"emb.field1": value1, ..., "emb.fieldN": valueN}

vicb avatar Dec 01 '20 19:12 vicb

I think this is a reasonable request. I did a quick compare with how this works in Python

Query projection: [<Entity('test', 'test_id') {'integer_column': 200}>]
Keys: dict_keys(['integer_column'])
Query no projection: [<Entity('test', 'test_id') {'integer_column': 200, 'field': 'field_value'}>]
Keys: dict_keys(['integer_column', 'field'])

It seems we keep it consistent there which points to there being a precedence for this. I think if we can do this in a non-breaking way I could be okay with this.

@bcoe can you weigh in from a more seasoned node perspective?

crwilcox avatar Dec 01 '20 22:12 crwilcox