Cursor & SelectMapper Refactor (WIP) #4180

benbjohnson · 2015-09-20T22:34:55Z

Overview

This pull request refactors the SelectMapper into separate raw and aggregate mappers and it refactors the Cursor implementation to return int64 keys and interface{} values.

This pull request is largely complete but still has test failures

otoolep · 2015-09-21T15:57:17Z

Can you explain why this will help with the upcoming engine changes? We started months ago with a single Mapper type, the first round of DQ changes split it into Raw and Aggregate, and @dgnorton merged it back to a single type. Now this change splits it again.

I'd like to make sure I have the full context.

dgnorton · 2015-09-21T16:36:19Z

I didn't merge raw and agg mappers. At least not that I remember. :)

corylanou · 2015-09-21T16:36:43Z

tsdb/engine/b1/b1.go

-func (tx *Tx) Cursor(key string, direction tsdb.Direction) tsdb.Cursor {
-	// Retrieve key bucket.
-	b := tx.Bucket([]byte(key))
+// Cursor returns an iterator for a key over a single field.


I may be reading this wrong, but we say single field here, do we mean single point? Later, in the cursor.read method we check to see if we have a single field or multiple fields, which implies we can cursor over multiple fields right?

The Cursor() was to do a single field originally. It was too hard to split that out right now so we'll have to wrap the pd storage with a wrapping cursor to join individual field cursors.

otoolep · 2015-09-21T16:40:41Z

@dgnorton -- yeah, I stated that incorrectly. What you did do is embed the Raw Mapper inside a LocalMapper i.e. set a Local into Raw mode. In other words, we're churning the implementation of this, and I wanted to understand why we're reverting to an earlier form. Perhaps it's for the same reasons I first separated the two Mappers.

benbjohnson · 2015-09-21T21:28:30Z

@otoolep I resolved some more test failures but it's really slow going. I'm getting a type assertion error now. It's difficult to keep track of which fields are getting processed where. I pushed my latest code (98328ab).

otoolep · 2015-09-21T21:37:54Z

OK @benbjohnson - I will take a look.

I was mistaken about work @dgnorton had done, I forgot that I merged the mappers.

corylanou · 2015-09-21T22:58:52Z

.hooks/pre-commit

@@ -7,7 +7,7 @@ if [ $fmtcount -gt 0 ]; then
 fi

 # Due to the way composites work, vet will fail for some of our tests so we ignore it
-vetcount=`go tool vet --composites=false ./ 2>&1  | wc -l`
+vetcount=`go tool vet --composites=false --methods=false ./ 2>&1  | wc -l`


Let me know if you have a better method name than Seek().

It probably makes sense to move to iterators (with just a Next() method) instead of cursors but that's a refactor for another time.

I hit this before and just used the name SeekTo.

Ah, I see you also used SeekTo, according to a comment below. So can this go vet change be pulled?

otoolep · 2015-09-22T18:30:34Z

tsdb/cursor.go

 	// Read items until we have a key that doesn't match the previously read one.
 	// This is to perform deduplication when there's multiple items with the same key.
 	// The highest priority cursor will be read first and then remaining keys will be dropped.
 	for {
-		// Return nil if there are no more items left.
+		// Return EOF marker if there are no more items left.


Why this change?

nil isn't allowed now that we're using int64 so I wanted to declare the marker more explicitly than simply doing -1.

otoolep · 2015-09-22T18:54:40Z

I took a look at this, and it generally looks fine.

Can you give us a couple of lines on your thinking, about why you wanted this change?

…apper Conflicts: tsdb/store.go

Cursor & SelectMapper Refactor (WIP)

benbjohnson · 2015-09-22T20:15:51Z

@otoolep Sure, the main goal for this change was to make it so that Tx.Cursor() could accept a field name instead of returning an encoded byte slice. That's because @pauldix's new engine is columnar-based and it handles encoding/decoding inside the engine. Unfortunately, breaking it into a single field was really difficult so it accepts a list of fields ([]string) instead. We'll have to wrap Paul's cursors in a parent cursor to handle multiple fields being grouped for the short term.

There were a few issues with passing fields into the cursor:

Decoding had to move from tsdb down into the individual engines. This is how it should have been from the beginning so it's good that we made this change. It's ultimately the engine's responsibility to determine how to handle encoding.
Cursor and mapper code was very, very tightly coupled in many spots. This caused me to split out the aggregate, raw, and remote mappers out separately. These could still use some refactoring still but they're much cleaner than they were.

otoolep · 2015-09-22T22:02:11Z

Thanks @benbjohnson -- both of those points are good ones, with which I agree.

benbjohnson added the 2 - Working label Sep 20, 2015

corylanou reviewed Sep 21, 2015
View reviewed changes

simplify select mapper stmt

0883182

otoolep reviewed Sep 22, 2015
View reviewed changes

benbjohnson added 6 commits September 22, 2015 13:09

refactor SelectMapper

1b8b625

rename direction to ascending.

a5269e9

refactor cursor

b213dda

fix tests

649663c

go vet: remove -methods

420b8ed

fix integration tests

56cb2fa

benbjohnson force-pushed the refactor-select-mapper branch from b99da42 to 56cb2fa Compare September 22, 2015 19:10

rename Cursor.Seek() to Cursor.SeekTo()

96715d7

benbjohnson force-pushed the refactor-select-mapper branch from 639db30 to 96715d7 Compare September 22, 2015 19:23

Merge remote-tracking branch 'upstream/master' into refactor-select-m…

8e27cf1

…apper Conflicts: tsdb/store.go

benbjohnson added a commit that referenced this pull request Sep 22, 2015

Merge pull request #4180 from benbjohnson/refactor-select-mapper

522f9ea

Cursor & SelectMapper Refactor (WIP)

benbjohnson merged commit 522f9ea into influxdata:master Sep 22, 2015

benbjohnson removed the 2 - Working label Sep 22, 2015

benbjohnson deleted the refactor-select-mapper branch September 22, 2015 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cursor & SelectMapper Refactor (WIP) #4180

Cursor & SelectMapper Refactor (WIP) #4180

benbjohnson commented Sep 20, 2015

otoolep commented Sep 21, 2015

dgnorton commented Sep 21, 2015

corylanou Sep 21, 2015

benbjohnson Sep 21, 2015

otoolep commented Sep 21, 2015

benbjohnson commented Sep 21, 2015

otoolep commented Sep 21, 2015

corylanou Sep 21, 2015

benbjohnson Sep 21, 2015

otoolep Sep 22, 2015

otoolep Sep 22, 2015

otoolep Sep 22, 2015

benbjohnson Sep 22, 2015

otoolep commented Sep 22, 2015

benbjohnson commented Sep 22, 2015

otoolep commented Sep 22, 2015

Cursor & SelectMapper Refactor (WIP) #4180

Cursor & SelectMapper Refactor (WIP) #4180

Conversation

benbjohnson commented Sep 20, 2015

Overview

otoolep commented Sep 21, 2015

dgnorton commented Sep 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otoolep commented Sep 21, 2015

benbjohnson commented Sep 21, 2015

otoolep commented Sep 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otoolep commented Sep 22, 2015

benbjohnson commented Sep 22, 2015

otoolep commented Sep 22, 2015