List:MySQL++« Previous MessageNext Message »
From:Warren Young Date:November 17 2008 10:20am
Subject:Re: Here goes nothing.
View as plain text  
On Nov 14, 2008, at 7:32 PM, Rick Gutleber wrote:
> Index: lib/manip.cpp
> ===================================================================
> --- lib/manip.cpp	(revision 2407)
> +++ lib/manip.cpp	(working copy)
> @@ -28,6 +28,7 @@
> #include "manip.h"
>
> #include "query.h"
> +#include "sqlstream.h"

This patch is malformed.  The first character of each patch line  
should be a space, a +, or a -.  Your mail client or mail server may  
be stripping the leading space.  Instead of pasting it into the  
message, try attaching it.  Better, redirect it out to a file, gzip or  
bzip2 that, and attach that.

> Index: lib/sqlstream.h
> ===================================================================
> --- lib/sqlstream.h	(revision 0)
> +++ lib/sqlstream.h	(revision 0)
> @@ -0,0 +1,140 @@
> +/// \file sqlstream.h
> +/// \brief Defines a class for building quoted and escaped SQL text.
> +
> +/ 
> ***********************************************************************
> + Copyright (c) 1998 by Kevin Atkinson, (c) 1999-2001 by MySQL AB, and

This should be (c) you or your organization, after you make the  
changes requested below.

> +	/// \brief Return a SQL-escaped version of a character buffer
> +	///
> +	/// \param ps pointer to C++ string to hold escaped version; if

...bla bla bla...

Rather than duplicate this comment, do a \sa to a common, single  
location.  Where to redirect -- that is, which location becomes  
"official" -- is a matter for you to decide...read on.

Ditto for the other variant of SQLStream::escape_string().

> Property changes on: lib/sqlstream.h
> ___________________________________________________________________
> Name: svn:executable
>   + *
> Name: svn:keywords
>   + Id
> Name: svn:eol-style
>   + native

These shouldn't go in the patch.

The executable flag is probably getting set because of Windows/*ix  
cross platform issues.  Say "svn propdel svn:executable lib/ 
sqlstream.h" to get rid of it.

We don't translate RCS/CVS-style keywords in MySQL++.  propdel  
svn:keywords, too.

The EOL thing is probably another cross-platform thing.  Just switch  
line endings until it goes away.  I'll fix up any remaining  
differences after applying the patch.

If you can't get rid of all the svn commentary in the patch, don't  
worry about it.  Either patch(1) will ignore it, or I can hack it out  
by hand.  We're just getting rid of the differences we *can* control  
up front.

> +SQLStream::SQLStream(Connection* c, const char* pstr) :
> +        std::ostringstream(),
> +        conn_(c)

Initializers aren't indented.

> +size_t
> +SQLStream::escape_string(std::string* ps, const char* original,
> +		size_t length) const

You may have been confused by seeing this.  Wrapped lines are double- 
indented, but ctor initializer lists don't count as wrapped lines.

Not sayin' it's consistent, just how it is.

Regarding the complete duplication of escape_string() in two places,  
you need to find a way around doing that.

The cleanest way may be to make them static methods of SQLStream, and  
call them from both locations.  If you do it this way, wrap the static  
methods in a DOXYGEN_IGNORE ifdef, as we don't want this as a  
documented feature.  The non-static versions would be the official  
versions of escape_string() in this case.

Another way would be to let Query::escape_string() continue to be the  
official version of these methods for now, and have SQLStream create  
temporary Query objects to pass the call through.  Fairy ugly, yes,  
but not as ugly as duplicate code.

Still another way is to investigate if we can't move most of it to  
DBDriver, and have *that* be the common location.

> Index: lib/insertpolicy.h
> ===================================================================
> --- lib/insertpolicy.h	(revision 0)
> +++ lib/insertpolicy.h	(revision 0)
> @@ -0,0 +1,221 @@
> +/// \file insertpolicy.h
> +/// \brief Declares the InsertPolicy classes.
> +///
> +/// These objects are used by the Query::insertfrom() method to  
> insert
> +/// collections of SSQLS objects when certain constraints in the raw
> +/// SQL command, such as a maximum are required.
> +
> +/ 
> ***********************************************************************
> + Copyright (c) 2008 by Educational Technology Resources, Inc.  Others

(c) you, again.  You've copied only trivial bits of code that we wrote.

> +	/// \brief stub to replace Transaction::commit()
> +	void commit() {
> +	}

MySQL++ style lets you save a line by putting both braces on the same  
line, with a space between:

	void commit() { }

That rule applies when you can get the whole function onto a single  
line, or at least its function body.  This is also legal:

	void commit_and_do_other_stuff(const with& these_things)
			{ implementation_of_function(); }

That works best when the prototype is long, but the body is a single  
statement and is short enough to fit on a single line.

If the body is multiple statements or you can't bring both braces up  
to the same line, they have to be on separate lines, in K&R style:

	void commit()
	{
	}

You have to do it this way for ctors, even if they have an empty body,  
since it's confusing to have "{ }" after the last initializer.

> +/// \brief The default policy object for doing bulk inserts.
> +///
> +/// InsertPolicy objects work hand-in-hand with Query::insertfrom().
> +///
> +/// The InsertPolicy object determines if another object can be added
> +/// to the INSERT statement using either internal state variables or
> +/// by looking at the length of the query, which is one of the
> +/// arguments passed in.
> +
> +/// This policy simply creates a single SQL statement regardless
> +/// of how big it gets.
> +template <class AccessController = Transaction>
> +class MYSQLPP_EXPORT DefaultInsertPolicy

Document AccessController here.

> +	/// \brief Can we add another object to the query?
> +	///
> +	/// \param size current length of the INSERT statement
> +	/// \param object the SSQLS object to be added
> +	///
> +	/// \retval true if the object is allowed to be added to the
> +	///     INSERT statement
> +	template <class RowT>
> +	bool can_add(int size, const RowT& object) const {
> +		if (size >= size_) {
> +			return false;
> +		}
> +
> +		SQLStream s(conn_);
> +
> +		s << ", (" << object.value_list() << ")";
> +
> +		size_t diff = size_ - size;
> +
> +		return (diff > s.str().size() );
> +	}

I'd express this as:

bool can_add(stuff)
{
	if (size < size_) {
		// Haven't hit size threshold yet, so see if this next
		// item pushes it over the line.
		SQLStream s(conn_);
		s << ", (" << object.value_list() << ")";
		return (size_ - size) >= s.str().size();
	}
	else {
		// Already too much in query buffer!
		return false;
	}
}

Main differences being that the if and else halves are more  
symmetrical, the common case is first, and there are no parens around  
a return value without good reason.  "return" isn't a function. :)

Also, check that the first return should indeed use >= rather than >.   
I think you might be shorting yourself one byte of buffer space.   
There shouldn't be anything after the ) of the final VALUES clause.

> -	/// \sa replace(), update()
> +	/// \sa replace(), update(), insertfrom()

Alphabetical order, please.

> +	template <class Iter, class InsertPolicy>
> +	Query& insertfrom(Iter first, Iter last, InsertPolicy& policy )
> +	{
> +		bool success = true,
> +			  empty = true;

Style again.  Either:

	bool success = true, empty = true;

or:

	bool success = true;
	bool empty = true;

There are other places this occurs.

> +					// at this point all we can do is give up
> +					if (throw_exceptions()) {
> +						throw BadInsertPolicy("Insert policy is too strict");
> +					}

Nice.  :)

> +		if ( success ) {

if (success) {, please.

There are other places this occurs.

> Index: test/qssqls.cpp
> ===================================================================
> --- test/qssqls.cpp	(revision 2407)
> +++ test/qssqls.cpp	(working copy)
> @@ -32,11 +32,8 @@
> using namespace std;
>
>
> -// Don't use any stringish types here.  That will cause code below to
> -// eventually try to call DBDriver::escape_string() through the
> -// Connection object, which we don't really have, so it asplodes.

I'm not sure what's going on here exactly.  Are you saying your patch  
fixes this, or that you noticed that the warning just isn't true any  
more?  I would have thought the latter, since without a connection,  
Query::escape_string() calls the static version of  
DBDriver::escape_string(), which should always succeed.  It's only the  
object method version that requires a connection.

Either way, this wants to be a separate patch.  It's not really  
anything to do with insertfrom().

> Index: test/qstream.cpp
> ===================================================================
> --- test/qstream.cpp	(revision 2407)
> +++ test/qstream.cpp	(working copy)
> @@ -7,7 +7,7 @@
>  Others may also hold copyrights on code in this file.  See the
>  CREDITS file in the top directory of the distribution for details.
>
> - This file is part of MySQL++.
> + This file is part of MySQL++.
>
>  MySQL++ is free software; you can redistribute it and/or modify it
>  under the terms of the GNU Lesser General Public License as published

Pointless whitespace change.

> +    o Query and SQLStream could have a common base class that would
> +      allow the stream manipulator functions to catch and modify
> +      strings based on only one dynamic_cast instead of requiring
> +      two as it does since the addition of the SQLStream class.  This
> +      could also help ease the unnecessary duplication of
> +      escape_string() methods.

The last sentence can go away once the escape_string() functionality  
exists in only one place.

> ---------------- BEGIN cgi_jpeg OUTPUT ----------------
> -Content-type: text/plain
> -
> -No image content!
> +Content-type: text/plain
> +
> +No image content!

Hmmm!  I think this may explain Michael O's problem.

> Index: examples/ssqls6.cpp
> ===================================================================
> --- examples/ssqls6.cpp	(revision 0)
> +++ examples/ssqls6.cpp	(revision 0)
> @@ -0,0 +1,155 @@
> +/ 
> ***********************************************************************
> + ssqls6.cpp - Example showing how to insert a collection row using  
> the
> + Specialized SQL Structures feature of MySQL++ and  
> Query::insertfrom().
> +
> + Copyright (c) 1998 by Kevin Atkinson, (c) 1999-2001 by MySQL AB, and
> + (c) 2004-2008 by Educational Technology Resources, Inc.  Others may

Add yourself to this list.

> +		if (tokenize_line(line, strings) != 6) {
> +			cerr << "Error parsing input line (doesn't have 6 fields) " <<
> +					"in file '" << filename << "'" << endl;
> +			input.close();
> +			return false;
> +		}
> +		
> +		row.item = strings[0];
> +		row.num = strings[1];
> +		row.weight = strings[2];
> +		row.price = strings[3];
> +		row.sdate = strings[4];
> +		row.description = strings[5];

I don't think a bad line should be fatal.  Also, you should be able to  
construct and append the row on a single line:

	if (tokenize_line(line, strings) == 6) {
		stock_vector.push_back(stock(strings[0], strings[1]...);
	}
	else {
		cerr << "Insert angst here";
	}

If you think the construct-and-insert is too tricky, do it on two lines:

	stock row(strings[0], strings[1]...);
	stock_vector.push_back(row);

The 7-line 'row' creation you have now is too verbose.

> +		// this policy means that after the INSERT statement exceeds
> +		// 1000 characters in length, it will be executed and a new on
> +		// started
> +		mysqlpp::SizeThresholdInsertPolicy<> insert_policy(1000);
> +
> +		query.insertfrom(stock_vector.begin(), stock_vector.end(),
> +				insert_policy);
> +
> +		query.insertfrom(stock_vector2.begin(), stock_vector2.end(),
> +				insert_policy );

I'm not seeing enough value in two separate reads and inserts.  It  
complicates things.  Instead, I'd rather just encourage people to  
experiment with different policies to see how it changes the  
insertion.  I'd keep the stock.txt dataset, as it's more diverse.  Or,  
merge the two.

The distributed version of ssqls6.cpp and stock.txt should cause at  
least two INSERTs.  If you can manage it, it'd be even better if it  
can do 3 inserts, with each testing a different path through  
insertfrom().  Ideally, at least one of these INSERTs should test an  
edge case, like a maximally-full packet.

You're really close!  This is all just cleanup.
Thread
Re: Here goes nothing.Warren Young17 Nov
  • Re: Here goes nothing.Rick Gutleber19 Nov
    • Re: Here goes nothing.Warren Young19 Nov
      • Re: Here goes nothing.Rick Gutleber19 Nov
        • Re: Here goes nothing.Jonathan Wakely20 Nov
      • Unexpected results with stock (was Re: Here goes nothing.)Rick Gutleber19 Nov
Re: Here goes nothing.Warren Young17 Nov