tarantool / cartridge-java

Tarantool Cartridge Java driver for Tarantool versions 1.10+ based on Netty framework
https://tarantool.io
Other
27 stars 11 forks source link

Работа с Null значениями #358

Closed ext0404 closed 1 year ago

ext0404 commented 1 year ago

Feature request from customer :

"tarantool позволяет использовать null значения в полях с типом Map, java-connector позволяет сохранять ключи с null-значением, но не позволяет их читать. Проблема в коде ниже:

public class DefaultMapValueToMapConverter implements ValueConverter> { ...

public Map fromValue(MapValue value) { return value.map().entrySet().stream() .filter(e -> !e.getValue().isNilValue()) // <<<<<<< в итоге ключа в мапе нет .collect(Collectors.toMap(e -> mapper.fromValue(e.getKey()), e -> mapper.fromValue(e.getValue()))); } }

очевидно, не следует использовать stream, т.к. он не позволяет работать с null"

akudiyar commented 1 year ago

Saving null values in the Java maps is considered a bad practice. Java maps are designed differently and the absence of a key means the same that the null value for that key. Also, the null value has an uncertain type, not related to the map's value type.

So, you need to write your Java code in a different way or have a custom converter that sets the null values. But having such a converter in the driver code looks controversial. However, we may consider providing an example of a custom converter here.

vlalykin commented 1 year ago

there is a mismatch between the tarantool server and the connector. Tarantool is not Java. Different connectors in different programming languages can work with this storage at the same time. in Java itself, having null in the map is normal (see HashMap).

now the behavior is such that the java client simply does not see some of the keys. this is an obvious mistake. Fix it - a matter of 30 seconds. it is strange to offer to write a custom converter, explaining this with some initially controversial practices

PS especially since the connector itself saves null values, and does not read them itself.

akudiyar commented 1 year ago

@vlalykin Java's HashMap can have nulls, but it is the same that not having a value. The conversion of a Lua map to a Java map is not 1-1 because the Java Map API and implementation differ. In Java connector, we map Tarantool structures to the Java types but do not try to follow the internal Tarantool or Lua behavior, because in some cases it is even not 1-1 reproducible (like in this one).

What do you mean by the connector does not "see" a value? Would you mind providing an example? How do you suppose to use these null values?

vlalykin commented 1 year ago

@akudiyar

Java's HashMap can have nulls, but it is the same that not having a value.

HashMap has the key, but doesn't have the null value. These are different things.

What do you mean by the connector does not "see" a value?

Connector doesn't see map elements, that contain the null value.


reproduce:

tarantoolctl:

format = {};
format[1] = {'id', 'unsigned'};
format[2] = {'map', 'map', is_nullable = true};
s = box.schema.create_space('test', {format = format});
_ = s:create_index('pk')
s:replace({1,setmetatable({}, {__serialize='map'})})
s:replace({2,setmetatable({}, {__serialize='map'})})
s:update( {1}, { {'=','map.a',box.NULL}, {'=','map.b',1} })
s:update( {2}, { {'=','map.a',box.NULL}, {'=','map.b',box.NULL} })

s:select()
---
- - [1, {'b': 1, 'a': null}]
  - [2, {'b': null, 'a': null}]
...

Java:

        client.space("test").select(Conditions.any()).get().forEach(
                tuple -> System.out.println(tuple.getLong("id") + " = " + tuple.getMap("map"))
        );
---
1 = {b=1}
2 = {}

but at the same time:

        Map map = new HashMap();
        map.put("a", null);
        map.put("b", 1);
        System.out.println(map);
---
{a=null, b=1}
vlalykin commented 1 year ago

@akudiyar

How do you suppose to use these null values?

use cases:

  1. it is required to store set of unique values. There is no SET type in tarantool space, so map field is used to store keys (value = null)
  2. Even in Java itself, it may be necessary to distinguish between the presence of a key and its absence. This is important, for example, when implementing JSON MERGE PATCH: RFC-7396.

    Null values in the merge patch are given special meaning to indicate the removal of existing values in the target.

There were other cases, but I can’t immediately remember

akudiyar commented 1 year ago

@vlalykin

HashMap has the key, but doesn't have the null value. These are different things.

Using nulls in Java Map leads to unexpected and unwanted behavior of the Map object. We will not do this in the driver code. If you wish to have that in your code - that's your choice. Here is a good article on what's happening with the Map object if you put null values into it and why we won't do this.

it is required to store set of unique values. There is no SET type in tarantool space, so map field is used to store keys (value = null)

You may return an array and use a Set on the Java side and that's a usual and obvious choice. But using a table with byte values (e.g. 0, 1, true, false) in Tarantool for the set representation is also possible and looks better than having nulls (box.NULL) as the value, and the result size of such MsgPack table will be the same. Using box.NULL looks like a strange pattern, on the other hand.

Even in Java itself, it may be necessary to distinguish between the presence of a key and its absence. This is important, for example, when implementing JSON MERGE PATCH: RFC-7396.

JSON is not a Java Map, it is a different structure and Java's Map was not designed with a look back at JSON. It is unlike Python's, Ruby's, JavaScript's, and even Lua (!) structures and shouldn't be confused with them. In Lua a nil for the key also represents its absence and for that reason, in Tarantool we have a kludge with box.NULL. For a JSON-like structure, you may use the corresponding type from a library like Jackson and a custom converter that saves null values.

Perhaps a built-in converter for the Jackson library types may be a good feature request for the built-in driver converters, but it's a separate ticket. And I'd rather keep the additional converters as additional plug-in libraries for the driver, otherwise the driver will be bloated with converters for all the possible types that may be useful. Our intention is to keep the built-in list of converters only for the built-in Java core types. So, if we'll have a built-in JSON type in Java in the future, we may consider adding a built-in converter for it, but it most likely will not be the Map type because of Java's eagerness for backward compatibility.

vlalykin commented 1 year ago

You may return an array and use a Set on the Java side

SET is not needed in java, it is needed on the server. The operations of adding a value to a set use its important feature: no duplicates are added, and no pre-checking of the contents is necessary. Obviously any array doesn't suit this. In this case, you do not need to write extra lua procedures, and json-path is used:

s:update( {1}, { {'=','map.a',box.NULL} } ) // [1, {'a': null}]
s:update( {1}, { {'=','map.a',box.NULL} } ) // [1, {'a': null}]

but if the map type is chose for storage in the server part, then of course on the client side, in whatever programming language it is implemented, we must get the same type

Using box.NULL looks like a strange pattern, on the other hand.

it's not strangely, it's natural. it would be strange to use any other value, because there is no value as such, only keys are needed

vlalykin commented 1 year ago

JSON is not a Java Map, it is a different structure and Java's Map was not designed with a look back at JSON. It is unlike Python's, Ruby's, JavaScript's, and even Lua (!) structures and shouldn't be confused with them. In Lua a nil for the key also represents its absence and for that reason, in Tarantool we have a kludge with box.NULL

About an empty key in lua - this is not the point. we are talking about the empty value JSON is not map, it is JSON -)

however, if Json is an object (not array), then the map represents it perfectly. The implementation of Java parsers allows you to get such maps where the key is not empty, but the value is empty.

Moreover, there are different implementations of JSON MERGE PATH, and they all use a map with null values. Because it is naturally. Similarly, http request headers are not maps, but all frameworks use the map type

vlalykin commented 1 year ago

We will not do this in the driver code. If you wish to have that in your code - that's your choice.

I can't force you. But:

akudiyar commented 1 year ago

box.NULL is not a normal value, it's a special value and not a part of the Lua language. It is being interpreted as Java's null intentionally because it is actually representing the Lua's nil value, and interpreting it differently just for the special Map case will introduce an inconsistency. That's why it is not natural to use box.NULL (it does not provide any benefits over true, false, 0 or 1) and why we cannot interpret it differently in the Map converter.

vlalykin commented 1 year ago

@akudiyar we are talking about the fact that the space has a field of type Map. And the key 'a' (or 'b') is stored in it. And it has no value.

s:select()
---
- - [1, {'b': 1, 'a': null}]
  - [2, {'b': null, 'a': null}]

Why such a value is stored - does not matter. It's legal. For normal fields, the connector interprets the absence of a value as null, and it should do the same here. "No value for a key" is a "key with a null value". Moreover, the existing code already interprets the absence of a value as null:

   assertNull(mapper.fromValue(ImmutableNilValueImpl.get())) // this is OK (mapper is the DefaultMessagePackMapper)

that is, all you need to do is replace

    public Map<?, ?> fromValue(MapValue value) {
        return value.map().entrySet().stream()
            .filter(e -> !e.getValue().isNilValue())
            .collect(Collectors.toMap(e -> mapper.fromValue(e.getKey()), e -> mapper.fromValue(e.getValue())));
    }

with:

    public Map<?, ?> fromValue(MapValue value) {
        Map<Object, Object> result = new HashMap<>();
        value.map().forEach((k, v) -> result.put(k, v));
        return result;
    }

I honestly don't understand what the problem is. There are no technical restrictions. Apparently, this is just a reluctance to work with such maps, but you do not need to work with them. This is user data. Give them to the user without loss and forget about it.

akudiyar commented 1 year ago

Why such a value is stored - does not matter. It's legal.

In the standard Java Map there is no such thing as the "presence of a key without a value". It is equivalent to the absence of a key and when you test for the presence of a key with a null value, you will get different results depending on the way you are doing it. The fact that Java Map (for historical reasons) allows storing a key with a null value (and that is not universal, in some cases it allows it and in some not, see the article mentioned above), does not mean that it is right to do this. A null value for a map key in Lua means effectively the same. Tarantool sets a special value for nulls in a tuple as a workaround and it logically means the same as Lua nil - an absence of a value for a given key. You are using that special value (box.NULL) in your own scenario, however, I do not recommend doing this, because box.NULL should not really be used anywhere outside of the Tarantool tuples.

vlalykin commented 1 year ago

Я не знаю, что еще сказать. не понимаю, почему такое сопротивление. Делаю последнюю попытку, на русском языке, чтобы избежать вероятности неправильного истолкования чего-либо.

"Легально" означает, что сервер tarantool хранит такие данные. Задача драйвера - предоставить данные с сервера на клиент без искажений, чтобы мы получали в точности то, что есть, и при обратном сохранении полученного на сервер получили там в точности то, что было. Сохранение сейчас работает как надо, а вот чтение - нарушает взаимно-однозначное соответствие в описанном мной случае. Происходит потеря ключа, хотя нет никаких технических проблем, чтобы предоставить этот ключ в мапе.

И я привожу только факты, никаких измышлений:

Про box.NULL и lua. Они использованы всего лишь в качестве примера (так проще сделать reproduce). Я использую tarantoolctl для демонстрации ошибок и в crud, и в tarantool-core, и в java-connector. И они прекрасно справляются с этой задачей. Тот же box.NULL используется в tarantoolctl для вставки "null" в любое другое поле, числовое или строковое. И это ни у кого не вызывает возражений. Мапу с ключом, но без значения, можно сохранить в space и с помощью другого языка программирования, и в том числе с использованием java-connector.

Цель данного issue - это устранение несоответствия .

akudiyar commented 1 year ago

Я не вижу смысла сохранять ключ в Java Map со значением null, если Java Map не поддерживает однозначно различие между наличием ключа с null значением и отсутствием ключа. Это ограничение реализации Map в Java, а не коннектора. Так как коннектор для языка Java, он должен соответствовать спецификациям языка, а не создавать ситуации с undefined behavior.

vlalykin commented 1 year ago

нет никакого ограничения. Интерфейс Map содержит метод containsKey, однозначно поддерживая отличие между этими двумя ситуациями.

vlalykin commented 1 year ago

ну и не надо ничего специально сохранять. там уже все есть. просто не надо его искусственно убирать.

akudiyar commented 1 year ago

@vlalykin ConcurrentHashMap, новые конструкторы для Map и в целом новые версии Java не будут поддерживать null-значения в Map, из-за того самого undefined behavior, когда map.get() возвращает null всегда, а map.containsKey() соответствует ему не всегда. См. https://openjdk.org/jeps/269. Такое поведение явно отмечено как нежелательное в спецификации. Вы можете сделать кастомный конвертер и использовать его, это просто - всего лишь надо взять дефолтный MessagePackMapper из конфига и модифицировать, добавив собственный конвертер для HashMap. Менять конвертер по умолчанию мы не будем, из-за уже озвученных причин и так как он много где используется.

Можно подумать о специальном конвертере MessagePack map -> Set, который заполняет Java Set ключами из таблицы. Такой вариант устроит?

vlalykin commented 1 year ago

@akudiyar

JEP 269: Convenience Factory Methods for Collections

это все о другом - "удобные фабричные методы". Null в HashMap и Map.containsKey никто не уберет как минимум из-за обратной совместимости. Вам же не надо работать с этой Map в своем SDK, ее надо только вернуть пользователю, а дальше - его дело. Но существует объективная реальность: данные на сервере tarantool могут быть такие! А java-connector не обеспечивает полной совместимости (хотя всё для этого есть)

Вы можете сделать кастомный конвертер

Конечно, я могу сделать свой конвертер, но это просто вопрос совместимости с сервером tarantool ! Как я уже писал, любой тест, просто сохраняющий мапу, а затем читающий ее обратно, покажет несовпадение данных. Ну а если с данными одновременно работает разное ПО, написанное на разных языках ? Мне кажется тут вообще не о чем спорить, но конечно остается вопрос обратной совместимости в java-connector:

Менять конвертер по умолчанию мы не будем, из-за уже озвученных причин и так как он много где используется.

вот тут я уже не могу знать наверняка (где и как используется). Но если вдуматься, то появление на стороне Java нового ключа с null-значением возможно только при наличии такого ключа на сервере. Кажется, что это обратно-совместимое изменение. Сложно представить, чтобы кто то сознательно хранил такие ключи в поле таблицы и полагался на такое неявное поведение, как исчезновение такого ключа на стороне java. То есть ни NPE, ни коллизии ни у кого из существующих потребителей из-за этого не возникнет. Но теоретически конечно можно за это зацепиться :)

Можно подумать о специальном конвертере MessagePack map -> Set, который заполняет Java Set ключами из таблицы.

А как будете сохранять такой Set из java в таблицу ? какое значение будете использовать в мапе на сервере ? если не null, то что тогда ? 0, 1, 100500 ? массив использовать нельзя, так как он не дает уникальности ключей. Думаю, что это будет не очень "чистое" поведение и вызовет дополнительные вопросы. Ну и главное здесь все же в том, что Set лишь частный случай (это мой частный кейс), а в общем случае может храниться Map как с null-значениями, так и с not null. В tarantool это будет совершенно законно, а на клиенте опять надо будет писать кастомный конвертер, Set здесь не поможет.

В общем, я думаю, дискуссию надо завершать. Вопрос очень простой, но превратился в настоящий холивар :) Все что мог, я уже сказал. Лично я могу жить с тем, что есть. Просто я хотел, чтобы система клиент-сервер была "полна", "симметрична" и т.п.

akudiyar commented 1 year ago

@vlalykin В коде на Lua (основной язык для работы с Тарантулом) никто не полагается на наличие ключей в таблице, если их значение равно nil. Т.е. поведение на стороне Lua соответствует поведению в Java. То, что такие ключи возвращаются из Тарантула - следствие того, что они хранятся в msgPack. Полагаться на такое поведение - это некий специальный кейс, который в реальных бизнес-приложениях использовать не рекомендуется.

akudiyar commented 1 year ago

@vlalykin

А как будете сохранять такой Set из java в таблицу ? какое значение будете использовать в мапе на сервере ?

Значение выбирается пользователем - можете хоть nil использовать, как вы хотите. Будет итерация только по ключам, которые есть в MsgPack map.

массив использовать нельзя, так как он не дает уникальности ключей.

Set.addAll() в Java работает таким образом, что исключает дубликаты. Нет проблем, если хотите получить Set из MsgPack array - будет Set.

в общем случае может храниться Map как с null-значениями, так и с not null.

Кроме этого частного случая мне трудно понять, зачем null значения в таблице, если они даже не экономят место в MsgPack и при этом с ними нельзя работать нормально в нескольких языках. Звучит как антипаттерн. Это не "законно" в бизнес-логике для Тарантула, и это просто артефакт хранения данных.

vlalykin commented 1 year ago

@akudiyar

В коде на Lua (основной язык для работы с Тарантулом) никто не полагается на наличие ключей в таблице, если их значение равно nil. Т.е. поведение на стороне Lua соответствует поведению в Java. То, что такие ключи возвращаются из Тарантула - следствие того, что они хранятся в msgPack. Полагаться на такое поведение - это некий специальный кейс, который в реальных бизнес-приложениях использовать не рекомендуется.

Я говорю только про данные в space на сервере. Это же первично. Естественно, это порождает следствия. Это просто "полнота" системы. Здесь не важно, какие кейсы. Зачем нужны в мапе null-ы - это решает конечный пользователь. Жизнь сложна, и кейсы всегда найдутся. Но здесь это оффтопик :) И я бы понял, если бы были технические проблемы в java, но ведь их нет. Да, конечно, можно сказать "не рекомендуется", но зачем искусственно урезать работающую функциональность ? Все же отлично работает. Просто нужно создать HashMap и все :)

P.S. Кстати, можно же в select указывать в условиях jsonpath выражения. Вот Вам еще один способ воспользоваться наличием или отсутствием ключа, даже не прибегая к услугам конкретного connector. На практике правда не проверял.

vlalykin commented 1 year ago

Set.addAll() в Java работает таким образом, что исключает дубликаты. Нет проблем, если хотите получить Set из MsgPack array - будет Set.

да я про сервер говорю, про тип поля. Речь идет о конкурентном добавлении ключей в множестве (а не о перезаписи множества целиком), с использованием jsonpath. Без предварительного анализа. AddAll в java здесь ничего не даст.