Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FLINK-5527][query] querying a non-existing key does not return the default value #3142

Closed
wants to merge 1 commit into from

Conversation

NicoK
Copy link
Contributor

@NicoK NicoK commented Jan 17, 2017

Querying for a non-existing key for a state that has a default value set currently results in an UnknownKeyOrNamespace exception when the MemoryStateBackend or FsStateBackend is used. It should return the default value instead just like the RocksDBStateBackend.

…efault value

Querying for a non-existing key for a state that has a default value set currently results in an UnknownKeyOrNamespace exception when the MemoryStateBackend or FsStateBackend is used. It should return the default value instead just like the RocksDBStateBackend.
@StephanEwen
Copy link
Contributor

In a separate change, we actually deprecated the default value in states. It does not behave very well at runtime in many cases and we thought that it would be simpler to let the user code handle default values.

Can queryable state not simply return null as well? I am also not sure if the assumption can be made that external queries want the same default value as the state updating code paths (which frequently use the default value only as a shortcut for initializing the state).

@NicoK
Copy link
Contributor Author

NicoK commented Jan 18, 2017

I saw that deprecation but nonetheless the default value is exposed which is why a consistent behaviour is needed.

Since the state descriptor says "that is the value if nothing is set", I would find it more consistent if that was true for both use cases of "state", i.e. inside an operator and as queryable state. It would make things more complicated for users if it were only true for the operator use, I guess.

If the default value is not used, however, it is set to null internally and so null is returned anyway only at the penalty of this one check that the actual value is null

@StephanEwen
Copy link
Contributor

The default value so far referred to access to the state object from within a program.
I would avoid extending this definition to queryable state given that we are deprecating it.

If we clearly spell the behavior out, I see no problem that queryable state access does not use the default value.

@uce
Copy link
Contributor

uce commented Jan 20, 2017

I agree with both of your points, but I'm slightly leaning towards the side of consistent behaviour since this behaviour is not special for queryable state per se. It's just implementing the current contract of the state interfaces, which still allow for a default value. Once we remove that from the state interfaces, queryable state should also not expose it anymore, since queryable state is registered within the program.

In the end I'm fine with both approaches and it's up to you both to decide. 😉

PS Note that null is not returned in case of a query to a non-existent value for a key/namespace, but the UnknownKeyOrNamespace exception fails the queries Future instance.

@NicoK
Copy link
Contributor Author

NicoK commented Jan 23, 2017

Ok, let's not introduce the (now deprecated) default values in the queryable state API.
I'll create a new Jira and PR for removing that part from the RocksDB back-end and consistently return null for now (which translates to the UnknownKeyOrNamespace exception).

Later we can decide to return null instead, if desired. Although a query for non-existent state could be considered a failure from the point of the queryable state API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants