[tree] major and minor indices to long64, it was forgotten to change from 32bit of old interfaces

[ferdymercury]

This Pull request:

Changes or fixes:

Fixes https://its.cern.ch/jira/browse/ROOT-9028

Checklist:

• [x] tested changes locally
• [x] updated the docs (if necessary)

[phsft-bot]

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default How to customize builds

[pcanal]

The concern on this patch is that due to the ( <<32 ) are practically limited to 32 bits inputs. Whenever the inputs goes past this, the result will become ambiguous. Related, does this fix the actual use (odd maybe unsupported) use case described in the report?

[ferdymercury]

The concern on this patch is that due to the ( <<32 ) are practically limited to 32 bits inputs. Whenever the inputs goes past this, the result will become ambiguous. Related, does this fix the actual use (odd maybe unsupported) use case described in the report?

Yep, I guess that if you use minor> 2^23 with major = 0, then this patch would solve it.

Also, it seems more consistent with the other function.

[github-actions[bot]]

Test Results

    21 files      21 suites   3d 7h 35m 42s ⏱️  3 211 tests  3 211 ✅ 0 💤 0 ❌ 65 699 runs  65 699 ✅ 0 💤 0 ❌

Results for commit 3ef05165.

:recycle: This comment has been updated with latest results.

[pcanal]

Yep, I guess that if you use minor> 2^23 with major = 0, then this patch would solve it.

I guess we should warn or error out if ( minor> 2^32 and major != 0) || (major> 2^32)

[ferdymercury]

@rdisipio do you have still by chance the ROOT file example from your post https://root-forum.cern.ch/t/ttree-getentrywithindex-error-due-to-cast-long-int-t/28316? That we could convert into a test.

[phsft-bot]

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default How to customize builds

[phsft-bot]

Build failed on windows10/default. See console output.

[phsft-bot]

Build failed on ROOT-ubuntu2004/python3. Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build See console output.

Errors:

• [2024-03-17T14:22:40.683Z] FAILED: tree/treeplayer/CMakeFiles/TreePlayer.dir/src/TTreeIndex.cxx.o
• [2024-03-17T14:22:41.253Z] /home/sftnight/build/workspace/root-pullrequests-build/root/tree/treeplayer/src/TTreeIndex.cxx:419:54: error: unable to find numeric literal operator ‘operator""ffffffff’
• [2024-03-17T14:22:41.253Z] /home/sftnight/build/workspace/root-pullrequests-build/root/tree/treeplayer/src/TTreeIndex.cxx:447:54: error: unable to find numeric literal operator ‘operator""ffffffff’

Warnings:

• [2024-03-17T14:22:41.253Z] /home/sftnight/build/workspace/root-pullrequests-build/root/tree/treeplayer/src/TTreeIndex.cxx:420:41: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 4 has type ‘Long64_t’ {aka ‘long long int’} [-Wformat=]
• [2024-03-17T14:22:41.253Z] /home/sftnight/build/workspace/root-pullrequests-build/root/tree/treeplayer/src/TTreeIndex.cxx:420:54: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 5 has type ‘Long64_t’ {aka ‘long long int’} [-Wformat=]
• [2024-03-17T14:22:41.253Z] /home/sftnight/build/workspace/root-pullrequests-build/root/tree/treeplayer/src/TTreeIndex.cxx:448:41: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 4 has type ‘Long64_t’ {aka ‘long long int’} [-Wformat=]
• [2024-03-17T14:22:41.253Z] /home/sftnight/build/workspace/root-pullrequests-build/root/tree/treeplayer/src/TTreeIndex.cxx:448:54: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 5 has type ‘Long64_t’ {aka ‘long long int’} [-Wformat=]

[phsft-bot]

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default How to customize builds

[phsft-bot]

Build failed on windows10/default. See console output.

[phsft-bot]

Build failed on ROOT-ubuntu2204/nortcxxmod. Running on root-ubuntu-2204-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build See console output.

Failing tests:

• projectroot.roottest.root.tree.index.roottest_root_tree_index_make

[phsft-bot]

Build failed on ROOT-performance-centos8-multicore/soversion. Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build See console output.

Failing tests:

• projectroot.roottest.root.tree.index.roottest_root_tree_index_make

[phsft-bot]

Build failed on ROOT-ubuntu2004/python3. Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build See console output.

Failing tests:

• projectroot.roottest.root.tree.index.roottest_root_tree_index_make

[phsft-bot]

Build failed on mac12arm/cxx20. Running on 194.12.161.128:/Users/sftnight/build/workspace/root-pullrequests-build See console output.

Failing tests:

• projectroot.roottest.root.tree.index.roottest_root_tree_index_make

[phsft-bot]

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default How to customize builds

[phsft-bot]

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default How to customize builds

[phsft-bot]

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default How to customize builds

[phsft-bot]

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default How to customize builds

[ferdymercury]

@vepadulano @pcanal I just noticed that all the 31 bit ugly stuff was due to an outdated documentation. It's no longer the case, minor and major have now independent variables both 64-bits. So this simplifies things significantly. See https://github.com/root-project/root/commit/19b0ca55fe1643eb07c9bca362b64a4d2111674f

[phsft-bot]

Build failed on ROOT-ubuntu2204/nortcxxmod. Running on root-ubuntu-2204-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build See console output.

Failing tests:

• projectroot.roottest.root.tree.index.roottest_root_tree_index_make

[phsft-bot]

Build failed on ROOT-performance-centos8-multicore/soversion. Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build See console output.

Failing tests:

• projectroot.roottest.root.tree.index.roottest_root_tree_index_make

[phsft-bot]

Build failed on mac12arm/cxx20. Running on 194.12.161.128:/Users/sftnight/build/workspace/root-pullrequests-build See console output.

Failing tests:

• projectroot.roottest.root.tree.index.roottest_root_tree_index_make

[phsft-bot]

Build failed on ROOT-ubuntu2004/python3. Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build See console output.

Failing tests:

• projectroot.roottest.root.tree.index.roottest_root_tree_index_make

[vepadulano]

I believe the current failures are related to the roottest branch not being up-to-date with the latest master and the recently merged https://github.com/root-project/root/pull/15021 , other than the failures already present in the sibling roottest PR at https://github.com/root-project/roottest/pull/1090. I don't see the failures on a PR based on the latest master commit https://github.com/root-project/root/pull/15184

[pcanal]

I believe the current failures are related to the roottest branch not being up-to-date with the latest master

Close but not quite. What is happening in the reverse. Once the CI test are started they pin the ROOT master commit that will be use to test. If one simply "re-run" the tests they will (this is intentional) use that same commit. To take in consideration new commits in the new CI build we need to do either of these 3 actions:

• Rebase the master onto the new PR branch.
• Add a new commit to the PR branch.
• Close and re-open the PR.

[ferdymercury]

fyi @TomasDado this PR is 'stalled' since I am not able to figure out a way to make the test pass in all platforms at the same time.

If this is urgently needed, the change 'just' in the source code could be in principle decoupled from the test in my opinion, since, even if the change is not perfect, it improves things.

[TomasDado]

Thanks for the ping @ferdymercury. Maybe it helps if I give some information to the best of my knowledge regarding this issue and ATLAS. We definitely have data periods where the eventNumber (msot commonly used for building the index) exceeds 2^32 thus we also store it as a 64 bit integer. However, I have never seen a need to build the index for data events, this this might not be an urgent issues, althought theoretically it is a problem for data. For MC, where the indexing functionality is definitely used, I am not aware of any sample exceeding 2^32 events although I think we are not that far (~1B events I have seen). Thus, I do not think the issue is urgent, but it might become urgent in the near future if we extend our MC samples.

[ferdymercury]

@pcanal I would suggest merging this PR 'as is'. The cross-platform compatibility limits of these functions have now been clearly documented, and the tests were simplified to the least common factor.

This improves the current situation, instead of using just a maximum of 32 bits, now we can use up to 52 bits, and the documentation informs you about that.