Changeset: f43530c3b667 for MonetDB
URL: https://dev.monetdb.org/hg/MonetDB/rev/f43530c3b667
Modified Files:
monetdb5/modules/mal/tablet.c
monetdb5/modules/mal/tablet.h
sql/backends/monet5/rel_bin.c
sql/backends/monet5/sql.c
sql/backends/monet5/sql_result.c
Branch: directappend
Log Message:
Cleanup, mostly comments
diffs (267 lines):
diff --git a/monetdb5/modules/mal/tablet.c b/monetdb5/modules/mal/tablet.c
--- a/monetdb5/modules/mal/tablet.c
+++ b/monetdb5/modules/mal/tablet.c
@@ -1570,7 +1570,7 @@ SQLload_file(Client cntxt, Tablet *as, b
lng lio = 0, tio, t1 = 0, total = 0, iototal = 0;
char name[MT_NAME_LEN];
- threads = 1;
+ // threads = 1;
/* TRC_DEBUG(MAL_SERVER, "Prepare copy work for '%d' threads col '%s' rec
'%s' quot '%c'\n", threads, csep, rsep, quote);*/
@@ -1580,7 +1580,6 @@ SQLload_file(Client cntxt, Tablet *as, b
.from_stdin = from_stdin,
.as = as,
.escape = escape, /* TODO: implement feature!!! */
- // .loadops = loadops,
.loadops = loadops,
};
@@ -1775,7 +1774,7 @@ SQLload_file(Client cntxt, Tablet *as, b
}
if (task.top[task.cur] && loadops) {
- /* while the worker threads are working, allocate rows
*/
+ /* claim rows in the table while waiting for the
workers to finish, */
str msg = loadops->claim(loadops->state,
task.top[task.cur], 0, NULL);
if (msg != MAL_SUCCEED) {
tablet_error(&task, BATcount(countbat),
lng_nil, lng_nil, msg, "SQLload_file");
diff --git a/monetdb5/modules/mal/tablet.h b/monetdb5/modules/mal/tablet.h
--- a/monetdb5/modules/mal/tablet.h
+++ b/monetdb5/modules/mal/tablet.h
@@ -50,7 +50,7 @@ typedef struct Column_t {
char quote; /* if set use this
character for string quotes */
const void *nildata;
int size;
- void *appendcol; /* temporary, can use
Columnt_t.extra in the future */
+ void *appendcol; /* temporary, can probably use
Columnt_t.extra in the future */
} Column;
/*
@@ -70,7 +70,8 @@ typedef struct Table_t {
BAT *complaints; /* lines that did not match the
required input */
} Tablet;
-
+// Callback interface to append the data directly instead of storing it in
intermediate BATs.
+// SQLload_file doesn't know how to manipulate the sql transaction
bookkeeping, caller does.
typedef str (*loadfile_claim_fptr)(void *state, size_t nrows, size_t ncols,
Column *cols[]);
typedef str (*loadfile_append_one_fptr)(void *state, size_t idx, const void
*data, void *col);
typedef BAT *(*loadfile_get_offsets_bat_fptr)(void *state);
diff --git a/sql/backends/monet5/rel_bin.c b/sql/backends/monet5/rel_bin.c
--- a/sql/backends/monet5/rel_bin.c
+++ b/sql/backends/monet5/rel_bin.c
@@ -4239,12 +4239,12 @@ table_update_stmts(mvc *sql, sql_table *
return SA_ZNEW_ARRAY(sql->sa, stmt *, *Len);
}
+// Call this from the debugger at any time to see how code generation proceeds.
void dump_code(int);
static struct {
MalBlkPtr mb;
int pos;
} dump_code_state;
-
void
dump_code(int starting_point)
{
@@ -4270,6 +4270,9 @@ dump_code(int starting_point)
dump_code_state.pos = stop;
}
+// This is basically a long list of reasons not to use the new direct append
+// code. The idea is that we'll gradually add support for more cases and
shorten
+// this list until it disappears.
static sql_exp*
can_use_appendfrom(sql_rel *rel)
{
@@ -4317,6 +4320,9 @@ can_use_appendfrom(sql_rel *rel)
projection = NULL;
incoming = p;
}
+ /* this seems to occur around foreign keys and it scares me */
+ if (projection && rel_is_ref(projection))
+ return NULL;
if (incoming == NULL) {
// happens for example with INSERT INTO foo VALUES (..),
@@ -4325,15 +4331,11 @@ can_use_appendfrom(sql_rel *rel)
}
if (incoming->op != op_table)
return NULL;
-
- /* this seems to occur around foreign keys and it scares me */
- if (projection && rel_is_ref(projection))
- return NULL;
if (rel_is_ref(incoming))
return NULL;
-
if (incoming->flag != TABLE_PROD_FUNC)
return NULL;
+
sql_exp *copy_from = incoming->r;
if (copy_from->type != e_func)
return NULL;
@@ -4401,6 +4403,8 @@ can_use_appendfrom(sql_rel *rel)
return copy_from;
}
+// Temporarily emit the MAL to call to sql.copy_from and aggr.count directly
+// from rel2bin_insert. This needs to move to rel2bin_exp.
static stmt *
rel2bin_directappend(backend *be, sql_rel *rel, list *refs, sql_exp *copyfrom)
{
@@ -4423,7 +4427,7 @@ rel2bin_directappend(backend *be, sql_re
list_append(l, arg_stmt);
}
- // Then we emit the call. Maybe there's a stmt_function for that..
+ // Then emit the call. Maybe there's a stmt_function for that..
InstrPtr append_instr = newFcnCallArgs(mb, sqlRef, copy_fromRef, 100);
setDestType(mb, append_instr, newBatType(TYPE_oid));
for (node *n = l->h; n; n = n->next) {
@@ -4444,6 +4448,7 @@ rel2bin_directappend(backend *be, sql_re
// will make some else free the arg_stmt's we created here.
s->op4.lval = l;
+ // Accumulate row counts.
be->rowcount = be->rowcount
? add_to_rowcount_accumulator(be, s->nr)
: s->nr;
@@ -4461,10 +4466,13 @@ rel2bin_insert(backend *be, sql_rel *rel
dump_code_state.mb = be->mb;
// dump_code(0);
+ // If can_use_appendfrom doesn't return NULL, short circuit to
+ // the temporary dedicated code generator
sql_exp *copyfrom = can_use_appendfrom(rel);
if (copyfrom != NULL) {
// later on we'll do this properly, passing an extra parameter,
- // for now we adjust an existing parameter
+ // for now we adjust an existing parameter.
+ // Very ugly, sorry
list *args = copyfrom->l;
sql_exp* arg7 =
args->h->next->next->next->next->next->next->next->next->data;
assert(arg7->type == e_atom);
diff --git a/sql/backends/monet5/sql.c b/sql/backends/monet5/sql.c
--- a/sql/backends/monet5/sql.c
+++ b/sql/backends/monet5/sql.c
@@ -2905,6 +2905,7 @@ mvc_import_table_wrap(Client cntxt, MalB
bool append_directly = false;
int besteffort = *getArgReference_int(stk, pci, pci->retc + 8);
if (besteffort >= 100) {
+ // this matches the temporary ugliness in rel2bin_insert
besteffort -= 100;
append_directly = true;
}
diff --git a/sql/backends/monet5/sql_result.c b/sql/backends/monet5/sql_result.c
--- a/sql/backends/monet5/sql_result.c
+++ b/sql/backends/monet5/sql_result.c
@@ -688,12 +688,15 @@ has_whitespace(const char *s)
return 0;
}
+// Callback functions and state struct to be passed to SQLload_file as LoadOps.
+// This is the code that claims row space and writes to it.
+// SQLload_file can call it if available but doesn't know how it works.
struct directappend {
mvc *mvc;
sql_table *t;
- BAT *all_offsets;
- BAT *new_offsets;
- BUN offset;
+ BAT *all_offsets; // all offsets ever generated
+ BAT *new_offsets; // as most recently returned by mvc_claim_slots.
+ BUN offset; // as most recently returned by mvc_claim_slots.
};
static void
@@ -744,6 +747,10 @@ directappend_claim(void *state_, size_t
{
str msg = MAL_SUCCEED;
+ // these parameters aren't used right now, useful if we ever also move
the
+ // old bunfastapp-on-temporary-bats scheme to the callback interface
+ // too, making SQLload_file fully mechanism agnostic.
+ // Then again, maybe just drop them instead.
(void)ncols;
(void)cols;
@@ -751,9 +758,8 @@ directappend_claim(void *state_, size_t
struct directappend *state = state_;
if (state->new_offsets != NULL) {
- // leftover from previous round. logic below counts on it not
being present.
- // we can change that but have to do so carefully.
- // for now just drop it
+ // Leftover from previous round, the logic below counts on it
not being present.
+ // We can change that but have to do so carefully. for now just
drop it.
BBPreclaim(state->new_offsets);
state->new_offsets = NULL;
}
@@ -770,14 +776,16 @@ directappend_claim(void *state_, size_t
goto bailout;
}
- // Append the batch to all_offsets
+ // Append the batch to all_offsets.
if (state->new_offsets != NULL) {
if (BATappend(state->all_offsets, state->new_offsets, NULL,
false) != GDK_SUCCEED) {
msg = createException(SQL, "sql.append_from",
SQLSTATE(3F000) "BATappend failed");
goto bailout;
}
} else {
- // is there a BATfunction for this?
+ // Help, there must be a BATfunction for this.
+ // Also, maybe we should try to make state->all_offsets a void
BAT and only
+ // switch to materialized oid's if necessary.
BUN oldcount = BATcount(state->all_offsets);
BUN newcount = oldcount + nrows;
if (BATcapacity(state->all_offsets) < newcount) {
@@ -802,6 +810,9 @@ directappend_claim(void *state_, size_t
// In the remainder of the function, 'state->newoffsets' holds
'front_count'
// positions if it exists, while another 'back_count' positions start at
// 'back_offset'.
+ //
+ // TODO this code has become a little convoluted as it evolved.
+ // Needs straightening out.
size_t front_count;
size_t back_count;
BUN back_offset;
@@ -831,6 +842,7 @@ directappend_claim(void *state_, size_t
back_count = nrows;
back_offset = state->offset;
}
+ state->offset = back_offset;
// debugging
(void)front_count;
@@ -847,7 +859,6 @@ directappend_claim(void *state_, size_t
// fprintf(stderr, "consecutive offsets: " BUNFMT " .. "
BUNFMT"\n", start, end);
// }
- state->offset = back_offset;
assert(msg == MAL_SUCCEED);
return msg;
@@ -865,6 +876,8 @@ directappend_get_offsets_bat(void *state
return state->all_offsets;
}
+// Currently we're appending the values one by one but we need to switch to
+// a bulk interface.
static str
directappend_append_one(void *state_, size_t idx, const void *const_data, void
*col)
{
@@ -1037,8 +1050,9 @@ mvc_import_table(Client cntxt, BAT ***ba
throw(IO, "sql.copy_from", SQLSTATE(HY013)
MAL_MALLOC_FAIL);
}
if (loadops) {
+ // Return a single result, the all_offsets BAT.
BAT *oids_bat = directappend_state.all_offsets;
- directappend_state.all_offsets = NULL; // or
we'd try to reclaim it later
+ directappend_state.all_offsets = NULL; //
otherwise we'll try to reclaim it later
BBPfix(oids_bat->batCacheid);
(*bats)[0] = oids_bat;
} else {
_______________________________________________
checkin-list mailing list
[email protected]
https://www.monetdb.org/mailman/listinfo/checkin-list
