[mkgmap-dev] [Patch] Error in BoundaryUtil

Tue Mar 13 11:40:49 GMT 2012

Hi WanMil,

okay, works fine for me.

Gerd

WanMil wrote
> 
> Gerd,
> 
> I have commited your patch plus the header length implementation. The 
> int for the record layout should give the format of the records not the 
> header. So I don't see a reason to change the record layout id if I just 
> want to add a field to the header and therefore I need the header length.
> 
> WanMil
> 
>> Hi Gerd,
>>
>> the length field is required to be able to add more fields to the header
>> in the future. Example (please don't nail me with the given length of
>> the fields - it's just an example):
>>
>> Header:
>> Format type: String: BND
>> Timestamp: long
>> | Header length: int: 8+4+10+4 = 26 bytes
>> | Record format: String: RAW (8 bytes)
>> | Record layout: int: 0x01 (4 bytes)
>> | mkgmap release: String: 2248 (10 bytes)
>> | Additional id: int: 0x45 (4 bytes)
>> ... Records ...
>>
>> Now think about three different reader versions all trying to read that
>> file:
>>
>> Reader "old" does not know about the new additional id. But it can read
>> until the mkgmap release and can skip the 4 bytes fo the additional id
>> (due to the known header length).
>>
>> Reader "current" expects exactly all fields until the additional id. It
>> reads 26 bytes and finishes.
>>
>> Reader "future" expects an additional field "Java version". But after
>> reading the additional id it can detect that the header does not contain
>> this new field "Java version". Therefore it can assume a default value
>> for that.
>>
>> If you don't use the header length you won't have the chance to add
>> anything to the header. (You've had the same problem with the legacy
>> format...)
>>
>> WanMil
>>
>>
>>> Hi WanMil,
>>>
>>> I don't see a need for the length field in the header, and I don't see
>>> how I could use it
>>> (I found no way to calculate how many bytes were removed from a stream)
>>>
>>> So I've coded this:
>>> Format type: String: BND
>>> Timestamp: long
>>> | Record format: String: (RAW, QUADTREE)
>>> | Record layout: int
>>> | mkgmap release: String
>>> ... Records ...
>>>
>>> Attached is the patch that implements this.
>>>
>>> Gerd
>>>
>>> > Date: Sun, 11 Mar 2012 21:02:56 +0100
>>> > From: wmgcnfg@
>>> > To: mkgmap-dev at .org
>>> > Subject: Re: [mkgmap-dev] [Patch] Error in BoundaryUtil
>>> >
>>> > > Hi WanMil,
>>> > >
>>> > >
>>> > >
>>> > > > Date: Sun, 11 Mar 2012 20:08:54 +0100
>>> > > > From: wmgcnfg@
>>> > > > To: mkgmap-dev at .org
>>> > > > Subject: Re: [mkgmap-dev] [Patch] Error in BoundaryUtil
>>> > > >
>>> > > > i Gerd,
>>> > > >
>>> > > > can you please explain in short what's the difference between the
>>> three
>>> > > > formats (my thinking):
>>> > > > * Legacy (original - int coords)
>>> > > > * Raw (float coords)
>>> > > > * Quadtree (admin level areas merged to the quadtree format)
>>> > >
>>> > > yes, correct
>>> > >
>>> > > >
>>> > > > loadQuadTree (Quadtree format?) => loadQuadTreeFromStream
>>> (Quadtree
>>> > > > format?) => readStreamRawFormat (Raw format?) => readArea (Legacy
>>> format)
>>> > > >
>>> > > > At least it is irritating to me if there is a method
>>> readStreamRawFormat
>>> > > > which reads raw and/or legacy format.
>>> > >
>>> > > I agree. I'll change that so that we have two methods
>>> > > readStreamLegacyFormat and readStreamRawFormat.
>>> >
>>> > Great! Maybe it makes also sense to put the detection of the format
>>> into
>>> > one method? In the moment there are a lot of rel.endsWith(...)
>>> checks in
>>> > the code.
>>> >
>>> > >
>>> > > >
>>> > > > I would also propose a change to the header of the new raw and
>>> quadtree
>>> > > > format. In the legacy format the first String in the file gives
>>> the
>>> > > > release of mkgmap. You added a "_raw" or "_quadtree" (?) to mark
>>> the new
>>> > > > formats. This is unclean but required to support the legacy
>>> format. But
>>> > > > legacy format support can be dropped in the future (I think at
>>> least in
>>> > > > the end of 2012). So to be open for future other changes it
>>> would be
>>> > > > good to have a kind of format identification. This could be added
>>> after
>>> > > > the creation time. Having this format identification future
>>> changes to
>>> > > > the format could be easily and much cleaner added.
>>> > >
>>> > > Yes, my problem with the legacy format was that it did not contain
>>> such
>>> > > a format identifier.
>>> > > I've worked a long time with programs on IBMs mainframe system
>>> (OS/390,
>>> > > z/OS).
>>> > > They typically use an eye catcher to identify the record type and a
>>> > > numeric field that
>>> > > identifies the record layout version. In some programs, they have
>>> also a
>>> > > length field.
>>> > > Old programs can read newer structures because the newer fields are
>>> only
>>> > > added
>>> > > at the end of a structure, and the length field allows to skip them.
>>> > > I think this is a good idea, but we probably don't have to support
>>> all
>>> > > kinds of
>>> > > versions in programs and files.
>>> > >
>>> > > So, here is my suggestion:
>>> > > instead of adding _raw or _quadtree to the 1st string I replace it
>>> with BND.
>>> > > Legacy format is identified because it doesn't contain this string.
>>> > > A new String field fileFormat after the timestamp identifies the
>>> file
>>> > > format,
>>> > > either RAW or QUADTREE.
>>> > > One more int field recId identifies the record layouts used (0 for
>>> this
>>> > > first version).
>>> > > Whenever a new format is needed, we change fileFormat, if only a new
>>> > > field is needed in e.g. the AREA section, we increase recId.
>>> > > All methods that read the file verify that they are able to
>>> process the
>>> > > recId
>>> > > or they stop with an FormatException .
>>> > > OK?
>>> >
>>> > Sounds good. But I propose two additional things:
>>> > 1. a header size or id (otherwise we will have the same problem
>>> again to
>>> > add an additional information to the header)
>>> > 2. Keep an mkgmap release identifier.
>>> >
>>> > So the new format would look like:
>>> > Format type: String: BND
>>> > Header size: int
>>> > | Record format: String: (RAW, QUADTREE)
>>> > | Record layout: int
>>> > | Timestamp: long
>>> > | mkgmap release: String
>>> > ... Records ...
>>> >
>>> > WanMil
>>> >
>>> >
>>> > >
>>> > > Gerd
>>> > >
>>> > > >
>>> > > > WanMil
>>> > > >
>>> > > > > Hi WanMil,
>>> > > > >
>>> > > > > hmm, we have two errors here. I can confirm that my patch
>>> doesn't solve
>>> > > > > all problems,
>>> > > > > it just allows again to read the old *.bnd format and the new
>>> quadtree
>>> > > > > format, but mkgmap
>>> > > > > fails to read the intermediate format produced by the
>>> preparers 1st
>>> > > pass.
>>> > > > >
>>> > > > > > Date: Sun, 11 Mar 2012 18:25:21 +0100
>>> > > > > > From: wmgcnfg@
>>> > > > > > To: mkgmap-dev at .org
>>> > > > > > Subject: Re: [mkgmap-dev] [Patch] Error in BoundaryUtil
>>> > > > > >
>>> > > > > > Hi Gerd,
>>> > > > > >
>>> > > > > > without your patch preparing boundaries works.
>>> > > > >
>>> > > > > No, it doesn't. It stops (crashes) without any error message
>>> when
>>> > > trying
>>> > > > > to read the *.bnd
>>> > > > > files produced in the 1st pass. You can verify this by looking
>>> at the
>>> > > > > header of
>>> > > > > the new *.bnd files, they have the suffix _raw, not _quadtree.
>>> > > > >
>>> > > > > The new patch fixes this, but not the missing error detection.
>>> > > > >
>>> > > > > I think we need a try/catch block somewhere, either in
>>> Preparer or
>>> > > > > BoundaryPreparer.
>>> > > > > Will you take care of this?
>>> > > > >
>>> > > > > Gerd
>>> > > > >
>>> > > > >
>>> > > > > > Using your patch I get the following exception.
>>> > > > > >
>>> > > > > > java.util.concurrent.ExecutionException:
>>> > > > > > java.lang.IllegalArgumentException: Ill
>>> > > > > > egal Capacity: -572471432
>>> > > > > > at java.util.concurrent.FutureTask$Sync.innerGet(Unknown
>>> Source)
>>> > > > > > at java.util.concurrent.FutureTask.get(Unknown Source)
>>> > > > > > at
>>> > > > > >
>>> uk.me.parabola.mkgmap.main.Preparer.runPreparer(Preparer.java:92)
>>> > > > > > at uk.me.parabola.mkgmap.main.Main.endOptions(Main.java:349)
>>> > > > > > at
>>> > > > > >
>>> uk.me.parabola.mkgmap.CommandArgsReader.readArgs(CommandArgsReader.ja
>>> > > > > > va:126)
>>> > > > > > at uk.me.parabola.mkgmap.main.Main.main(Main.java:114)
>>> > > > > > Caused by: java.lang.IllegalArgumentException: Illegal
>>> Capacity:
>>> > > > > -572471432
>>> > > > > > at java.util.ArrayList.<init>(Unknown Source)
>>> > > > > > at
>>> > > > > >
>>> uk.me.parabola.mkgmap.reader.osm.boundary.BoundaryUtil.readStreamRawF
>>> > > > > > ormat(BoundaryUtil.java:282)
>>> > > > > > at
>>> > > > > >
>>> uk.me.parabola.mkgmap.reader.osm.boundary.BoundaryUtil.loadQuadTreeFr
>>> > > > > > omStream(BoundaryUtil.java:459)
>>> > > > > > at
>>> > > > > >
>>> uk.me.parabola.mkgmap.reader.osm.boundary.BoundaryUtil.loadQuadTree(B
>>> > > > > > oundaryUtil.java:145)
>>> > > > > > at
>>> > > > > >
>>> uk.me.parabola.mkgmap.reader.osm.boundary.BoundaryUtil.loadQuadTree(B
>>> > > > > > oundaryUtil.java:120)
>>> > > > > > at
>>> > > > > >
>>> uk.me.parabola.mkgmap.reader.osm.boundary.BoundaryPreparer$QuadTreeWo
>>> > > > > > rker.call(BoundaryPreparer.java:236)
>>> > > > > > at
>>> > > > > >
>>> uk.me.parabola.mkgmap.reader.osm.boundary.BoundaryPreparer$QuadTreeWo
>>> > > > > > rker.call(BoundaryPreparer.java:221)
>>> > > > > > at java.util.concurrent.FutureTask$Sync.innerRun(Unknown
>>> Source)
>>> > > > > > at java.util.concurrent.FutureTask.run(Unknown Source)
>>> > > > > > at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown
>>> > > > > > Source)
>>> > > > > > at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown
>>> > > > > > Source)
>>> > > > > > at java.lang.Thread.run(Unknown Source)
>>> > > > > >
>>> > > > > > My mkgmap call is:
>>> > > > > > java -jar mkgmap.jar --max-jobs=4
>>> > > > > > --createboundsfile=africa-boundaries.osm.gz
>>> > > --bounds=bounds/africa *.osm
>>> > > > > >
>>> > > > > > WanMil
>>> > > > > >
>>> > > > > > > Hi WanMil,
>>> > > > > > >
>>> > > > > > > sorry, one of my last patches for the performance branch
>>> corrupted
>>> > > > > > > BoundaryUtil.
>>> > > > > > > It was no longer able to read the legacy *.bnd format (also
>>> created
>>> > > > > in the
>>> > > > > > > preparer) :-(
>>> > > > > > >
>>> > > > > > > Attached is the patch.
>>> > > > > > >
>>> > > > > > > Gerd
>>> > > > > > >
>>> > > > > > >
>>> > > http://gis.19327.n5.nabble.com/file/n5554370/BoundaryUtil.java.patch
>>> > > > > > > BoundaryUtil.java.patch
>>> > > > > > >
>>> > > > > > > --
>>> > > > > > > View this message in context:
>>> > > > >
>>> > >
>>> http://gis.19327.n5.nabble.com/Patch-Error-in-BoundaryUtil-tp5554370p5554370.html
>>>
>>> > > > > > > Sent from the Mkgmap Development mailing list archive at
>>> > > Nabble.com.
>>> > > > > > > _______________________________________________
>>> > > > > > > mkgmap-dev mailing list
>>> > > > > > > mkgmap-dev at .org
>>> > > > > > > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>>> > > > > >
>>> > > > > > _______________________________________________
>>> > > > > > mkgmap-dev mailing list
>>> > > > > > mkgmap-dev at .org
>>> > > > > > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>>> > > > >
>>> > > > >
>>> > > > > _______________________________________________
>>> > > > > mkgmap-dev mailing list
>>> > > > > mkgmap-dev at .org
>>> > > > > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>>> > > >
>>> > > > _______________________________________________
>>> > > > mkgmap-dev mailing list
>>> > > > mkgmap-dev at .org
>>> > > > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>>> > >
>>> > >
>>> > > _______________________________________________
>>> > > mkgmap-dev mailing list
>>> > > mkgmap-dev at .org
>>> > > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>>> >
>>> > _______________________________________________
>>> > mkgmap-dev mailing list
>>> > mkgmap-dev at .org
>>> > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>>>
>>>
>>> _______________________________________________
>>> mkgmap-dev mailing list
>>> mkgmap-dev at .org
>>> http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>>
> 
> _______________________________________________
> mkgmap-dev mailing list
> mkgmap-dev at .org
> http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
> 

--
View this message in context: http://gis.19327.n5.nabble.com/Patch-Error-in-BoundaryUtil-tp5554370p5560709.html
Sent from the Mkgmap Development mailing list archive at Nabble.com.