Not possible to implement batch(ops) in terms of chained batch

[ronag]

I would expect to be able to implement batch(ops) in terms of a chained batch. However, that is not possible given the combination of abstract chained batch and the tests, i.e.

  _batch (operations, options, callback) {
    const batch = this.batch()
    for (const op of operations) {
      if (op.type === 'del') {
        if ('key' in op) {
          batch.del(op.key, op)
        }
      } else if (op.type === 'put') {
        if ('key' in op && 'value' in op) {
          batch.put(op.key, op.value, op)
        }
      }
    }
    batch.write(options, callback)
  }

Will fail two tests:

not ok 688 should be deeply equivalent
    ---
      operator: deepEqual
      expected: |-
        [ { type: 'put', key: 456, value: 99, custom: 123 } ]
      actual: |-
        [ { type: 'put', key: '456', value: '99', custom: 123, keyEncoding: 'utf8', valueEncoding: 'utf8' } ]
      at: RocksLevel.<anonymous> (/Users/ronagy/GitHub/rocksdb/node_modules/abstract-level/test/batch-test.js:299:9)
      stack: |-
        Error: should be deeply equivalent
            at Test.assert [as _assert] (/Users/ronagy/GitHub/rocksdb/node_modules/tape/lib/test.js:314:54)
            at Test.bound [as _assert] (/Users/ronagy/GitHub/rocksdb/node_modules/tape/lib/test.js:99:32)
            at Test.tapeDeepEqual (/Users/ronagy/GitHub/rocksdb/node_modules/tape/lib/test.js:555:10)
            at Test.bound [as same] (/Users/ronagy/GitHub/rocksdb/node_modules/tape/lib/test.js:99:32)
            at RocksLevel.<anonymous> (/Users/ronagy/GitHub/rocksdb/node_modules/abstract-level/test/batch-test.js:299:9)
            at RocksLevel.emit (node:events:527:28)
            at /Users/ronagy/GitHub/rocksdb/node_modules/abstract-level/abstract-chained-batch.js:134:27
            at processTicksAndRejections (node:internal/process/task_queues:82:21)
    ...
  not ok 690 plan != count
    ---
      operator: fail
      expected: 2
      actual:   3
      at: RocksLevel.<anonymous> (/Users/ronagy/GitHub/rocksdb/node_modules/abstract-level/test/batch-test.js:299:9)
      stack: |-
        Error: plan != count
            at Test.assert [as _assert] (/Users/ronagy/GitHub/rocksdb/node_modules/tape/lib/test.js:314:54)
            at Test.bound [as _assert] (/Users/ronagy/GitHub/rocksdb/node_modules/tape/lib/test.js:99:32)
            at Test.fail (/Users/ronagy/GitHub/rocksdb/node_modules/tape/lib/test.js:408:10)
            at Test.bound [as fail] (/Users/ronagy/GitHub/rocksdb/node_modules/tape/lib/test.js:99:32)
            at Test.assert [as _assert] (/Users/ronagy/GitHub/rocksdb/node_modules/tape/lib/test.js:400:14)
            at Test.bound [as _assert] (/Users/ronagy/GitHub/rocksdb/node_modules/tape/lib/test.js:99:32)
            at Test.tapeDeepEqual (/Users/ronagy/GitHub/rocksdb/node_modules/tape/lib/test.js:555:10)
            at Test.bound [as same] (/Users/ronagy/GitHub/rocksdb/node_modules/tape/lib/test.js:99:32)
            at RocksLevel.<anonymous> (/Users/ronagy/GitHub/rocksdb/node_modules/abstract-level/test/batch-test.js:299:9)
            at RocksLevel.emit (node:events:527:28)
    ...

[ronag]

Is there a way to fix this? Either in the abstract base class or modifying the tests?

[vweevers]

I would advise against that pattern. Going from a private API to a public API means double encoding, massaging of options, deferred open, etc. This will hurt performance and API consistency. The two forms of batch() also have different performance characteristics; combining them makes it difficult for a consumer to predict what's best for them.

[ronag]

Do you have any benchmarks on the two forms of batch for e.g. leveldb? Just curious.

[vweevers]

I don't have benchmarks. Personally I prefer batch(ops) because the array of operations is easier to integrate with other code. Chained batch could be useful when you're writing a big batch and want to avoid blocking the main thread on the copying of that data - but I've never had that problem.

[ronag]

Would it maybe make sense then to implement chained batch in terms of batch(ops) by default?

[vweevers]

It already does.

[vweevers]

https://github.com/Level/abstract-level/blob/45738328661a6235f74f75833b8f2c139b881bfb/lib/default-chained-batch.js#L32