I suppose the common case to optimize for with ErrorLogMap is the case when it's empty. So, the gain from switching to unordered_map may not be worth the trouble of losing the determinism of the output order. So, I will preserve the existing behavior for now unless someone else makes a strong case against it.
On Tue, Jul 24, 2018 at 12:33 PM, Todd Lipcon <[email protected]> wrote: > I suppose the advantage of it being ordered is that at least the output > order is deterministic. That is to say, if a new warning shows up, it won't > re-shuffle the ordering of all prior warnings. And we'll get the same > results regardless of whether we're using libstdcxx or libc++, which might > make test assertions easier. > > -Todd > > On Tue, Jul 24, 2018 at 12:26 PM, Sailesh Mukil < > [email protected]> wrote: > > > The ErrorLogMap's key is "TErrorCode::type" which is an Enum. I don't > think > > we can derive any inherent meaning from the order of the errors listed in > > the enum (other than OK=0, which wouldn't be in this map anyway). I'm not > > aware of any clients relying on this order, and even if there were, it > > would probably be incorrect to do so. So, unless anyone else chimes in > with > > a good reason to keep this as a map instead of an unordered_map, I'm all > > for changing it. > > > > On Tue, Jul 24, 2018 at 12:12 PM, Michael Ho <[email protected]> > > wrote: > > > > > Hi, > > > > > > While making some changes for be/src/util/error-util.cc, I stumbled > upon > > > the question of whether we need a map instead of an unordered map for > > > ErrorLogMap (used for tracking error messages for each error code). > This > > > affects the order in which error messages are printed in > PrintErrorMap(). > > > Looking through the code, it doesn't appear to me that there is any > > > ordering requirement. However, it's unclear to me if any client of > Impala > > > makes assumption about the ordering of the output in PrintErrorMap(). > So, > > > sending this email to the list in case anyone knows anything. > > > > > > -- > > > Thanks, > > > Michael > > > > > > > > > -- > Todd Lipcon > Software Engineer, Cloudera > -- Thanks, Michael
