Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Fix Version/s: 1.1.7, 1.2.0 beta 2
    • Component/s: None
    • Labels:
      None

      Description

      get_paged_slice doesn't reset the start column filter for the second returned row sometimes. So instead of getting a slice:

      row 0: <start_column>...<last_column_in_row>
      row 1: <first column in a row>...<last_column_in_row>
      row 2: <first column in a row>...

      you sometimes get:

      row 0: <start_column>...<last_column_in_row>
      row 1: <start_column>...<last_column_in_row>
      row 2: <first column in a row>...

      1. 4816-2.txt
        4 kB
        Sylvain Lebresne
      2. 4816-3.txt
        8 kB
        Piotr Kołaczkowski

        Activity

        Piotr Kołaczkowski created issue -
        Hide
        Sylvain Lebresne added a comment -

        My bet would be that this is due to the memtable iterator. The way we implement get_paged_slice, we reset the start of the QueryFilter used after the first row have been read. Which means that for each row, we need to make sure the filter is not used until we've processed the preceding row. This is the case for the sstable iterator, but not for the memtable one. Attaching patch to change that (I haven't tested the patch though).

        Show
        Sylvain Lebresne added a comment - My bet would be that this is due to the memtable iterator. The way we implement get_paged_slice, we reset the start of the QueryFilter used after the first row have been read. Which means that for each row, we need to make sure the filter is not used until we've processed the preceding row. This is the case for the sstable iterator, but not for the memtable one. Attaching patch to change that (I haven't tested the patch though).
        Sylvain Lebresne made changes -
        Field Original Value New Value
        Attachment 4816.txt [ 12549323 ]
        Sylvain Lebresne made changes -
        Attachment 4816.txt [ 12549323 ]
        Hide
        Sylvain Lebresne added a comment -

        Scratch that patch, brain fart. The AbstractIterator.computeNext() is called no sooner than on hasNext() so we should be good on that front. Still don't know what is the problem here.

        Show
        Sylvain Lebresne added a comment - Scratch that patch, brain fart. The AbstractIterator.computeNext() is called no sooner than on hasNext() so we should be good on that front. Still don't know what is the problem here.
        Hide
        Sylvain Lebresne added a comment -

        Ok, second attempt. Looking more closing it does seem that this is a problem with the interaction of the mergeIterator and the SSTableScanner. Basically the mergeIterator always needs to know what his "the next row" (during the reducing phase). If that next row was the one that the reducer ended up returning, we were fine (so with 1 sstable or if all sstables had the same rows, it was ok), but otherwise it might end up using the QueryFilter before it should for our get_paged_slice "hack".

        Anyway, all that the mergeIterator needs during its reduction phase is to know the next key. So attaching a patch that delay the use of the filter until the row data is actually queried.

        Show
        Sylvain Lebresne added a comment - Ok, second attempt. Looking more closing it does seem that this is a problem with the interaction of the mergeIterator and the SSTableScanner. Basically the mergeIterator always needs to know what his "the next row" (during the reducing phase). If that next row was the one that the reducer ended up returning, we were fine (so with 1 sstable or if all sstables had the same rows, it was ok), but otherwise it might end up using the QueryFilter before it should for our get_paged_slice "hack". Anyway, all that the mergeIterator needs during its reduction phase is to know the next key. So attaching a patch that delay the use of the filter until the row data is actually queried.
        Sylvain Lebresne made changes -
        Attachment 4816-2.txt [ 12549345 ]
        Hide
        Piotr Kołaczkowski added a comment - - edited

        Attaching 3rd version of the patch. LazyColumnIterator is used both for SSTableScanners and Memtable.
        This version works for me.

        Show
        Piotr Kołaczkowski added a comment - - edited Attaching 3rd version of the patch. LazyColumnIterator is used both for SSTableScanners and Memtable. This version works for me.
        Piotr Kołaczkowski made changes -
        Attachment 4816-3.txt [ 12549461 ]
        Piotr Kołaczkowski made changes -
        Assignee Sylvain Lebresne [ slebresne ]
        Sylvain Lebresne made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Reviewer jbellis
        Fix Version/s 1.1.7 [ 12323354 ]
        Hide
        Cathy Daw added a comment -

        +1 this version fixes my tests as well.

        Show
        Cathy Daw added a comment - +1 this version fixes my tests as well.
        Hide
        Sylvain Lebresne added a comment -

        To be clear, I'm also good on version 3 but I'll let Jonathan review since I've wrote version 2 on which version 3 is based.

        Show
        Sylvain Lebresne added a comment - To be clear, I'm also good on version 3 but I'll let Jonathan review since I've wrote version 2 on which version 3 is based.
        Jonathan Ellis made changes -
        Assignee Sylvain Lebresne [ slebresne ] Piotr Kołaczkowski [ pkolaczk ]
        Fix Version/s 1.2.0 beta 2 [ 12323284 ]
        Affects Version/s 1.1.0 [ 12317615 ]
        Affects Version/s 1.1.6 [ 12323257 ]
        Priority Blocker [ 1 ] Major [ 3 ]
        Reviewer jbellis slebresne
        Hide
        Jonathan Ellis added a comment -

        LGTM, committed

        Show
        Jonathan Ellis added a comment - LGTM, committed
        Jonathan Ellis made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Gavin made changes -
        Workflow no-reopen-closed, patch-avail [ 12729957 ] patch-available, re-open possible [ 12753519 ]
        Gavin made changes -
        Workflow patch-available, re-open possible [ 12753519 ] reopen-resolved, no closed status, patch-avail, testing [ 12756718 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Patch Available Patch Available
        17h 42m 1 Sylvain Lebresne 17/Oct/12 09:04
        Patch Available Patch Available Resolved Resolved
        7d 11h 44m 1 Jonathan Ellis 24/Oct/12 20:48

          People

          • Assignee:
            Piotr Kołaczkowski
            Reporter:
            Piotr Kołaczkowski
            Reviewer:
            Sylvain Lebresne
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development